@microsoft-github-policy-service agree

Hi

@hediet Hello! I see you are now assigned to this PR. A colleague of yours requested some changes and I've worked on a fix which I just published. You might want to take a look at that. :)

@hediet @aeschli Has any of you had any time to take a look? If you need me to review anything, refactor code or either rebase or merge my branch, let me know.

Thanks for the PR!
Two things: Columns are generally 1 based in VS Code.
Also, I doubt a different value than 1 would ever make sense for lineCommentTokenColumn, so I think that property should be refactored into a boolean.

Are you sure there is no way to do that with existing functionality? E.g. a regexp that matches the \n character? @aiday-mar might know more here

@hediet Thank you for the response. I agree! A boolean value would much probably be enough, I'll refactor that and add a commit.

Also, I've taken a good look at the source code which handles this kind of line comment action, and I don't see a more optimal way of accomplishing this with already existing functionality. You mentioned using a regexp that matches '\n', but I don't see how that would be useful to address the issue. Could you clarify a little further?

@aiday-mar any help is welcome, of course. :)

Could you clarify a little further?

I'm not very familiar with this code, but I remember other situations where that helped. Could be very well that it doesn't apply here!

I understand. Out of my head I don't think that would help address this case, but I'll take another look regardless. I'll post a commit once the code is refactored.

Hi @Enzo-Nunes thanks for the PR. I see you added a new field to the language configuration file called the lineCommentTokenColumn to define a custom value for the index at which to insert the comment. I wonder if we should be introducing such a field as this is makefile specific? I wonder if there are other languages that require comment insertion at a specific index? Letting @aeschli comment here

@aiday-mar Hello! Although this is makefile specific, @aeschli instructed me to not to hard code "makefile" into the line comment action code. As such, I've changed the fix to solve the issue through the language configuration service with that new lineCommentTokenColumn field.

While this means that this new field becomes available to every language, its usage is completely optional. The only place where this value is indeed set is in the language-configration.json of makefiles. This solution addresses the issue for makefiles and can also be used by other languages by changing their own configuration files to include this new field.

As @hediet suggested, I'll be refactoring the fix to only include a boolean value which indicates if the comment token should be placed adjacent to the leftmost column or not, resulting in an even simpler solution. I'll publish a commit later today.

@hediet I've refactored the fix into a boolean value, as requested. Also, you mentioned columns are 1 based, but since we're no longer using a number I assume that's not a problem anymore.

Let me know if you want any more changes to be made or if it's time to rebase.

I doubt a different value than 1 would ever make sense for lineCommentTokenColumn, so I think that property should be refactored into a boolean.

@hediet @aiday-mar

the language here requires the comment token to be in the 2nd column
microsoft/vscode-discussions#2363
it may work if the comment token is set to " #"

@Enzo-Nunes can you test if setting the comment token to ' #' results in ctrl+/ continuing to work correctly?
(with # in the 2nd column)

@Enzo-Nunes can you test if setting the comment token to ' #' results in ctrl+/ continuing to work correctly? (with # in the 2nd column)

It does! If you set the field lineCommentTokenFirstColumn to 'true' and set the comment token to something like ' #', it toggles comments correctly while placing the comment token on the 2nd column.

@aeschli @hediet have you guys had the time to take a look? Do you agree with the solution?

Hi @Enzo-Nunes Thank you for the ping. I will discuss this idea with my team. I am not sure if we should be modifying the language configuration file schema to support this case. I will let you know what we discuss.

Alright! I'll be waiting. Thank you for your support.

Hi @Enzo-Nunes thanks for the PR. I have not yet received word from my colleagues. I will bring it up this week or next in a meeting.

Thank you very much for the attention! I'll be waiting for new developments.

Hi @Enzo-Nunes yes apologies. I discussed this PR with my colleagues and they suggested the following. Perhaps we should change the field:

lineComment?: string | null;
to :

lineComment?: string | { comment: string; noIndent?: boolean } | null

This way we do not add a new field but extend the existing field so it can also be of type object that takes as an option the noIndent boolean. This boolean should by default be false. To make this cleaner, I would extract the object type into a separate interface.

Alright! I'll make those changes and get back to you.

@aiday-mar I've applied the changes you requested and the code looks even cleaner now!

I've also had to update the branch so everything compiled correctly on my end, so I had to force-push the rebase.

If there is anything else you want changed or clarified, let me know.

Thank you!

Hi @Enzo-Nunes thanks for the consistent work! I will have a look.

Hi @Enzo-Nunes I have had a look and will push some minor polish changes which do not change the logic. I will put an approval but I think we should merge this next week. This week we are preparing our next release and only critical changes are going into the code base.

@aiday-mar Alright, I understand. Thank you very much for the time and assistance. :)

@aiday-mar I see this is a contribution from the community and since we are in endgame week, is this required to be fixed, given that the bug it is fixing is in backlog and old

Edit: Ok, I see it is planned for June and Auto merge is disabled. I came across this in the code-review channel and therefore raised the flag.

Hi @sandy081 yep, I plan to merge this next week during debt week

And with 1 simple language configuration change, it breaks probably lots of comment extensions that rely on the lineComment being a string. (I was using the String.prototype.includes() to check for a specific comment type which now throws an error when encountering makefile language). Luckily I have a fix for it, but 100s of extensions potentially have been affected.

VSCode really should provide a getLanguageConfiguration()
we already have setLanguageConfiguration()