feat(runtime)!: add padding to commentstrings #14843
Conversation
ribru17
force-pushed
the
cms_space
branch
4 times, most recently
from
ca660b3
to
ade5306
Compare
|
Thanks for doing that work. I am not sure, it is a bit intrusive and not sure if all runtime file maintainers appreciate it, if we change some values behind their back. I also noticed that there are apparently 2 syntax script, that set the commentstring option. I think it sholdn't be set there. This clearly belongs into a ftplugin. We have done such a mass change before with @dkearns browsefilter change, but perhaps we should still try to contact at least the maintainers that appear to be active. Ping @tpope @romainl @gpanders @Freed-Wu @jamessan @hcs42 @dkearns @nickeb96 @justinmk @zzzyxwvut @vito-c @habamax @ObserverOfTime @benknoble @rhysd Also can you please amend your commit and add the reason for your change in the last Change Header? So instead of : Make it like this: @dkearns whats your opinion on such a mass change? |
|
I'm okay. Actually, I have been doing just that in my "after" ftplugins for years. +1 from me. |
|
Well, it might be overridden (and probably would be) when the next update for the plugin would be merged in. Like, gdscript or odin. I guess the same is for other maintainers having source code in separate repos. What is the purpose of the change? Apart of aesthetic? |
|
LGTM but want to hear from @echasnovski too |
|
Yeah, looks good to me too. The only thing that might need an extra attention is whether 'commentstring' with a space does not represent a comment in a particular filetype. As in "#aaa" is comment but "# aaa" is not. |
|
I generally favour this style and would also prefer that we improved consistency across the runtime files where possible. However, we've traditionally left this entirely to the discretion of the maintainers. It might be a good time to rethink that policy more generally. Excluding the whitespace makes some sense when the template string is used for a The value change does introduce some small issues with existing files. E.g., deleting a fold in a C file created with the old value only deletes the fold marker text and not the entire comment. This might eventually be categorised as a bug. This PR will probably annoy some users and possibly some maintainers but I suspect more are annoyed by the current variation across file types. It seems reasonable to me. I'm happy with the change to the ftplugins I maintain, I was just matching the default value format. Like @romainl, I modify this locally even for my published ftplugins. |
|
There are many maintainers that do not follow on a regular basis vim_dev or github prs. What is the plan to push these changes upstream to respected repositories? What should be done if new version of ft/plugin is sent to vim_dev to be included where commentstring is not changed? What is the purpose of adding padding to commentstring? |
I think that is the sticking point here: how much consistency (and implied ownership) the Vim project wants in their runtime files. Right now, there's a sliding scale between "fully owned" and "we import an upstream plugin as-is", which is semi-indicated by the "maintainer" line (and word of mouth). Personally, I would favor more consistency and oversight, but that of course implies (significant) additional maintenance work. Obviously that's a decision the project maintainers must make (and document) and beyond the scope of this PR. One option could be to farm this out into a separate "filetype files" repository that does the integration work and from which Vim syncs regularly (inspired by the -- very successful -- vim/colorschemes project, and what vim-polyglot should have been IMO); this is also how treesitter queries are handled by Neovim. One drawback would be that we're back at big "update runtime files" blobs, and I much prefer the current "fun-size" runtime file commits. |
|
I've handled the Debian related files in #14849. |
|
Unify if you're sure that 4/5 are in the right and I'll merge these changes whenever I have my own to apply. |
|
Added the
I tested each file I changed to make sure the syntax is still valid with a space, it looks to be the case for all of them. Though my testing was loose, just a quick read of the language spec (if available) and brief github search
Besides aesthetic, just consistency basically. A question from myself: should this PR add a small documentation note saying that space-padding is prefered in |
|
I am going to answer to several diferent points mentioned, see below. I think the TLDR is, this change seems appreciated by most maintainers and I am happy to merge those filetype changes, but please revert the changes to the new default value of the 'commentstring' option.
I think it is mainly consistency and aesthtics for commenting plugins. The hope is to prevent overwriting on next runtime update is that either during review we notice the unwanted change or the author notices it directly when creating a PR with the updates. However there are no guarantees, that we won't un-intentionally revert it... :/ Are you okay with a change to your maintained runtime files?
@ribru17 mentioned he checked that, at least briefly. So I hope this wouldn't happen...
That is a good point. I tested it briefly with
I am not so much a fan of this, because it won't solve the issue of us making neccessary(?) changes to the filetype plugins, but adds additional burden to merge those new repo every now and then (and we typically only get complaints once it has been merged into the main repo (or nowadays once you have merged it down to Neovim). So for now I am slightly negative to it, sorry.
So please remove the Debian related changes from this PR.
Sorry, I did not get this one. Finally, to my own point:
Can you please remove those (or add as a simple ftplugin instead)? And finally, for my own maintained ftplugin (xml), I'll make a related change to my xml repository. But please keep the change here, I am not planning to PR my changes anytime soon anyhow. I'll leave this open for a bit, so that others have a chance to comment (e.g. if they like to have it done as part of their regular runtime file updates). |
|
Ah, our comments crossed:
Yes, that is a good idea, to mention the intent is to have consistency and therfore the value should be space-padded. |
related: vim/vim#14843 Signed-off-by: Christian Brabandt <cb@256bit.org>
|
Ok, I have removed the default value change and also removed the changes in debian files 👍 And mentioned the space padding in the docs EDIT: Should I explicitly give space padding to commentstrings for files that use |
Sorry about that 😔
No worries, it's your project, and you do an admirable job maintaining it (if you don't mind me saying that). I just wanted to put the option on the table, so to speak. |
|
LGTM :) |
@chrisbra, the fact that a fifth of collected data reveals |
Sure. Hopefully it wouldn't be overridden in the future. I mainly was asking about the purpose as it wasn't clear who/what would benefit from it. vim-commentary works(or at least worked) with current commentstring variations just fine, adding paddings where user expects it. |
|
This option is also used for the addition of fold markers, which doesn't add whitespace padding automatically. |
I don't see it mentioned anywhere in the thread, so I'll call out the elephant in the room: The recently released Neovim 0.10 includes built-in For markdown.vim, I only omitted spaces because that's what html.vim did. I'm happy so see a unilateral fix for For liquid.vim, the tag-like nature of it makes me think it's one of the few where the space makes it look worse? Not a hill I'm going to die on, but I think I'd recommend leaving it alone. For pdf.vim, I don't even maintain my own repository for that. Change it, please, save me the trouble of sending an update. |
|
It may be necessary to first consider if having 'commentstring' as a string template is insufficient for its use cases. Some languages may have context-dependent comment formats (e.g. HTML, Vim script, JSX, TSX), but 'commentstring' can only specify one comment format. |
60% at least. I retract my position.
I mean keeping the change would be doing me a favor. For the rest I will update my repos when this PR is merged. The changes won't get overwritten in a future update, and the advisory comments in those files aren't necessary (but not hurting anything). |
|
I see some force-pushes: do you intend to squash the fixups into the original commit before this is merged? (It looks like you're using |
|
Yeah, I was doing fixups to make the intermediate diffs more transparent; I'll squash them at the end if that makes things easier for you all 👍 |
ribru17
force-pushed
the
cms_space
branch
2 times, most recently
from
cd27b24
to
a102857
Compare
|
I am also fine with this change |
|
Seems we only got approvals here. If you could do me one more favor please?
Yes, let's please also make this consistent here and add explicitly padding. Once this is done, please squash your changes into a single commit please. I am ready to include this then, thanks all! |
The default value is really just the value appropriate for C, like many of Vim's default option values. As we're updating all the ftplugins that explicitly use the default, including the C ftplugin, I don't see much point in keeping the current unpadded value. |
Changes many ftplugin files to add space padding in their comment strings, if possible.
|
alright then. I'll fix this up while merging. Thanks all |
…ftplugins
Problem: no whitespace padding in commentstring option in ftplugins
Solution: Change default to include whitespace padding, update
existing filetype plugins with the new default value
(Riley Bruins)
closes: vim/vim#14843
vim/vim@0a08306
Co-authored-by: Riley Bruins <ribru17@hotmail.com>
…ftplugins
Problem: no whitespace padding in commentstring option in ftplugins
Solution: Change default to include whitespace padding, update
existing filetype plugins with the new default value
(Riley Bruins)
closes: vim/vim#14843
vim/vim@0a08306
Co-authored-by: Riley Bruins <ribru17@hotmail.com>
…ftplugins
Problem: no whitespace padding in commentstring option in ftplugins
Solution: Change default to include whitespace padding, update
existing filetype plugins with the new default value
(Riley Bruins)
closes: vim/vim#14843
vim/vim@0a08306
Co-authored-by: Riley Bruins <ribru17@hotmail.com>
…ftplugins
Problem: no whitespace padding in commentstring option in ftplugins
Solution: Change default to include whitespace padding, update
existing filetype plugins with the new default value
(Riley Bruins)
closes: vim/vim#14843
vim/vim@0a08306
Co-authored-by: Riley Bruins <ribru17@hotmail.com>
…ftplugins
Problem: no whitespace padding in commentstring option in ftplugins
Solution: Change default to include whitespace padding, update
existing filetype plugins with the new default value
(Riley Bruins)
closes: vim/vim#14843
vim/vim@0a08306
Co-authored-by: Riley Bruins <ribru17@hotmail.com>
…ftplugins
Problem: no whitespace padding in commentstring option in ftplugins
Solution: Change default to include whitespace padding, update
existing filetype plugins with the new default value
(Riley Bruins)
closes: vim/vim#14843
vim/vim@0a08306
Co-authored-by: Riley Bruins <ribru17@hotmail.com>
|
@ribru17 @chrisbra This pull request left out the space before old-style Vim comments in ftplugin/vim.vim. Was that deliberate? (I'm happy to make a PR if it was a mistake.) |
|
Yes, please read the review comments |
|
@zeertzjq Thanks (and sorry for the noise). I scanned the main comments, but missed the review comments. |
Changes the default 'commentstring' value from '/%s/' to '/* %s */', and changes many ftplugin files to add space padding in their comment strings, if possible.
I understand if this PR is too invasive, and if needed I can amend it to only include ftplugin changes, without changing the default value of
commentstring. Using this query, we can see that about a fifth of the specifiedcommentstrings in variousftplugins are not given space padding, while the rest are. This PR is an attempt to standardize the space padding more (when possible in the language). The only one I didn't touch was COBOL, because it is already kind of wonky and column-specific. Of the ones I did change, I gave a quick spec-read and github code search to make sure that the space-padded comments are still valid syntax in that language. Sorry for the large review!