[vim/vim] feat(runtime)!: add padding to commentstrings (PR #14843)
Riley Bruins
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 specified commentstrings in various ftplugins 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!
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/14843
Commit Summary
- ce12228 feat(runtime)!: add padding to commentstrings
File Changes
(60 files)
- M runtime/doc/options.txt (2)
- M runtime/ftplugin/abaqus.vim (3)
- M runtime/ftplugin/asm.vim (3)
- M runtime/ftplugin/astro.vim (7)
- M runtime/ftplugin/bitbake.vim (3)
- M runtime/ftplugin/cgdbrc.vim (3)
- M runtime/ftplugin/csh.vim (3)
- M runtime/ftplugin/deb822sources.vim (3)
- M runtime/ftplugin/debsources.vim (3)
- M runtime/ftplugin/desktop.vim (3)
- M runtime/ftplugin/dtd.vim (3)
- M runtime/ftplugin/dtrace.vim (5)
- M runtime/ftplugin/erlang.vim (5)
- M runtime/ftplugin/eruby.vim (3)
- M runtime/ftplugin/fennel.vim (3)
- M runtime/ftplugin/fish.vim (3)
- M runtime/ftplugin/fortran.vim (3)
- M runtime/ftplugin/fstab.vim (3)
- M runtime/ftplugin/gdb.vim (3)
- M runtime/ftplugin/groovy.vim (3)
- M runtime/ftplugin/hamster.vim (3)
- M runtime/ftplugin/html.vim (3)
- M runtime/ftplugin/initex.vim (3)
- M runtime/ftplugin/java.vim (3)
- M runtime/ftplugin/javascript.vim (3)
- M runtime/ftplugin/jq.vim (3)
- M runtime/ftplugin/jsonc.vim (3)
- M runtime/ftplugin/ld.vim (3)
- M runtime/ftplugin/liquid.vim (3)
- M runtime/ftplugin/lisp.vim (3)
- M runtime/ftplugin/markdown.vim (3)
- M runtime/ftplugin/mma.vim (3)
- M runtime/ftplugin/modula2.vim (3)
- M runtime/ftplugin/modula3.vim (3)
- M runtime/ftplugin/nroff.vim (4)
- M runtime/ftplugin/obse.vim (3)
- M runtime/ftplugin/ocaml.vim (4)
- M runtime/ftplugin/odin.vim (3)
- M runtime/ftplugin/openvpn.vim (3)
- M runtime/ftplugin/pascal.vim (3)
- M runtime/ftplugin/pdf.vim (3)
- M runtime/ftplugin/perl.vim (3)
- M runtime/ftplugin/php.vim (3)
- M runtime/ftplugin/ps1.vim (3)
- M runtime/ftplugin/ps1xml.vim (3)
- M runtime/ftplugin/qml.vim (3)
- M runtime/ftplugin/racket.vim (3)
- M runtime/ftplugin/raku.vim (3)
- M runtime/ftplugin/rust.vim (3)
- M runtime/ftplugin/scdoc.vim (3)
- M runtime/ftplugin/scheme.vim (3)
- M runtime/ftplugin/tcl.vim (3)
- M runtime/ftplugin/typescript.vim (3)
- M runtime/ftplugin/vim.vim (5)
- M runtime/ftplugin/wat.vim (3)
- M runtime/ftplugin/xml.vim (3)
- M runtime/syntax/lc.vim (3)
- M runtime/syntax/mma.vim (3)
- M src/optiondefs.h (2)
- M src/testdir/gen_opt_test.vim (2)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Riley Bruins
@ribru17 pushed 1 commit.
- 8d718f9 feat(runtime)!: add padding to commentstrings
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Riley Bruins
@ribru17 pushed 1 commit.
- abcdb17 feat(runtime)!: add padding to commentstrings
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Riley Bruins
@ribru17 pushed 1 commit.
- ca660b3 feat(runtime)!: add padding to commentstrings
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Riley Bruins
@ribru17 pushed 1 commit.
- ade5306 feat(runtime)!: add padding to commentstrings
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
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
can you please indicate if you are okay with the suggested change?
Also can you please amend your commit and add the reason for your change in the last Change Header? So instead of :
" 2024 Jan 14 by Vim Project (browsefilter)
" 2024 May 24 by Riley Bruins <rib...@gmail.com>
Make it like this:
" 2024 Jan 14 by Vim Project (browsefilter)
" 2024 May 24 by Riley Bruins <rib...@gmail.com> ('commentstring')
@dkearns whats your opinion on such a mass change?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Romain Lafourcade
I'm okay. Actually, I have been doing just that in my "after" ftplugins for years. +1 from me.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Maxim Kim
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?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
ObserverOfTime
@ObserverOfTime approved this pull request.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Justin M. Keyes
LGTM but want to hear from @echasnovski too
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Evgeni Chasnovski
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
dkearns
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 foldmarker but obviously less so when utilised by comment plugins and similar. I assume the latter case was the motivation for this change?
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Maxim Kim
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?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
dkearns
@dkearns requested changes on this pull request.
In runtime/ftplugin/abaqus.vim:
> @@ -3,6 +3,7 @@ " Maintainer: Carl Osterwisch <cost...@gmail.com> " Last Change: 2022 Oct 08 " 2024 Jan 14 by Vim Project (browsefilter) +" 2024 May 23 by Riley Bruins <rib...@gmail.com>⬇️ Suggested change
-" 2024 May 23 by Riley Bruins <rib...@gmail.com> +" 2024 May 23 by Riley Bruins <rib...@gmail.com> (commentstring)
Can you please make this change to all files. It might help grab the maintainer's attention.
Depending on your editing skills you might want to wait until a decision has been made to accept the PR.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Clason
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.
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
James McCoy
I've handled the Debian related files in #14849.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
Unify if you're sure that 4/5 are in the right and &cms is
relevant (i.e. fdm=marker vs fdm=syntax).
I'll merge these changes whenever I have my own to apply.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Ajit Thakkar
Riley Bruins
@ribru17 pushed 1 commit.
- 7aca887 fixup! feat(runtime)!: add padding to commentstrings
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Riley Bruins
Added the ('commentstring') note, and also moved the syntax changes to appropriate ftplugin ones (one of the syntax files had it set in both so I didn't need to create a new ftplugin file).
The only thing that might need an extra attention is whether 'commentstring' with a space does not represent a comment in a particular filetype.
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
What is the purpose of the change? Apart of aesthetic?
Besides aesthetic, just consistency basically.
A question from myself: should this PR add a small documentation note saying that space-padding is prefered in commentstrings?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
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.
What is the purpose of the change? Apart of aesthetic?
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?
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.
@ribru17 mentioned he checked that, at least briefly. So I hope this wouldn't happen...
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.
That is a good point. I tested it briefly with fold.c from this repository. Vim seems to handle it gracefully, even so my 'commentstring' was set to /*%s*/ and I could successfully delete foldmarkers created with // %s. But still I think we shouldn't change the default 'commentstring' option.
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
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.
I've handled the Debian related files in #14849.
So please remove the Debian related changes from this PR.
Unify if you're sure that 4/5 are in the right and &cms is relevant (i.e. fdm=marker vs fdm=syntax).
Sorry, I did not get this one.
Finally, to my own point:
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.
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).
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Ah, our comments crossed:
A question from myself: should this PR add a small documentation note saying that space-padding is prefered in commentstrings?
Yes, that is a good idea, to mention the intent is to have consistency and therfore the value should be space-padded.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Riley Bruins
@ribru17 pushed 1 commit.
- 8e84beb fixup! fixup! feat(runtime)!: add padding to commentstrings
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Riley Bruins
Ok, I have removed the default value change and also removed the changes in debian files 👍 And mentioned the space padding in the docs
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Clason
(or nowadays once you have merged it down to Neovim).
Sorry about that 😔
So for now I am slightly negative to it, sorry.
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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
vito-c
LGTM :)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
Unify if you're sure that 4/5 are in the right and
&cmsis
relevant (i.e.fdm=markervsfdm=syntax).
Sorry, I did not get this one.
@chrisbra, the fact that a fifth of collected data reveals
a different preference for &cms doesn't say much in the
matter of idiosyncrasy (or policy), like tabs or spaces for
indentation. Besides, there is no poll to look at for how
many folks have their &fdm set to marker for this setting
to matter at all.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Maxim Kim
Are you okay with a change to your maintained runtime files?
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
This option is also used for the addition of fold markers, which doesn't add whitespace padding automatically.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
dkearns
@dkearns commented on this pull request.
In runtime/ftplugin/abaqus.vim:
> @@ -3,6 +3,7 @@ " Maintainer: Carl Osterwisch <cost...@gmail.com> " Last Change: 2022 Oct 08 " 2024 Jan 14 by Vim Project (browsefilter) +" 2024 May 23 by Riley Bruins <rib...@gmail.com>
Oops, missed the earlier comment on this. Thanks.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Tim Pope
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.
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 gc map that was inspired by commentary.vim, but which lacks the whitespace normalization. I'm quite sure that's the underlying motivation for this change.
For markdown.vim, I only omitted spaces because that's what html.vim did. I'm happy so see a unilateral fix for <!--%s--> in particular get considered; HTML comments without a space are especially ugly.
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
@zeertzjq commented on this pull request.
> else - setlocal commentstring=\"%s + setlocal commentstring=\"\ %s
Using a space here may not always work, as mentioned in :h line-continuation-comment.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
zeertzjq
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Csaba Hoch
I'm happy with this change. The folding markers look better this way.
(I am the maintainer of erlang.vim. I will update the vim-erlang repository after this PR gets merged.)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
I will take a detailed look this week, just FYI.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Riley Bruins
@zeertzjq I agree. It would be nice (but maybe super breaking) if it could take a function as well maybe. The astro ftplugin uses a CursorMoved autocmd to get around this, but I think that is not an ideal solution.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Riley Bruins
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.
I think I agree, but it seems like many inline comments for liquid have space padding (around 60% of the Github examples)
For pdf.vim, I don't even maintain my own repository for that. Change it, please, save me the trouble of sending an update.
Sorry, do you mean to revert the PDF change in this PR? pdf.vim is one of the files I changed already :)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Riley Bruins
@ribru17 pushed 1 commit.
- d9b6980 fixup! fixup! feat(runtime)!: add padding to commentstrings
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Tim Pope
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.
I think I agree, but it seems like many inline comments for liquid have space padding (around 60% of the Github examples)
60% at least. I retract my position.
For pdf.vim, I don't even maintain my own repository for that. Change it, please, save me the trouble of sending an update.
Sorry, do you mean to revert the PDF change in this PR? pdf.vim is one of the files I changed already :)
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).
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
@benknoble commented on this pull request.
In runtime/ftplugin/racket.vim:
> @@ -5,6 +5,7 @@ " URL: https://github.com/benknoble/vim-racket " Last Change: 2022 Aug 29 " 2024 Jan 14 by Vim Project (browsefilter) +" 2024 May 23 by Riley Bruins <rib...@gmail.com> ('commentstring')
I've made the requested change upstream; please correct the header to match:
" Vim filetype plugin
" Language: Racket
" Maintainer: D. Ben Knoble <ben.knob...@gmail.com>
" Previous Maintainer: Will Langstroth <wi...@langstroth.com>
" URL: https://github.com/benknoble/vim-racket
" Last Change: 2024 May 28
(Neither commentstring nor browsefilter updates need mentioned here.)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Riley Bruins
@ribru17 pushed 1 commit.
- e5f2d4c fixup! fixup! feat(runtime)!: add padding to commentstrings
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
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 commit --fixup, so rebase --autosquash should do the trick.)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Riley Bruins
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 👍
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Nicholas Boyle
I am also fine with this change
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Sebastian Lyng Johansen
@seblj commented on this pull request.
> @@ -51,7 +52,7 @@ setlocal keywordprg=:help
" Comments starts with # in Vim9 script. We have to guess which one to use.
if "\n" .. getline(1, 32)->join("\n") =~# '\n\s*vim9\%[script]\>'
- setlocal commentstring=#%s
+ setlocal commentstring=#\ %s
else
setlocal commentstring=\"%s
Should this also have padding?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Riley Bruins
@ribru17 commented on this pull request.
> @@ -51,7 +52,7 @@ setlocal keywordprg=:help
" Comments starts with # in Vim9 script. We have to guess which one to use.
if "\n" .. getline(1, 32)->join("\n") =~# '\n\s*vim9\%[script]\>'
- setlocal commentstring=#%s
+ setlocal commentstring=#\ %s
else
setlocal commentstring=\"%s
I did add it initially, but @zeertzjq pointed out that sometimes a backslash must immediately follow the " character, and having a space after would break things
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Sebastian Lyng Johansen
@seblj commented on this pull request.
> @@ -51,7 +52,7 @@ setlocal keywordprg=:help
" Comments starts with # in Vim9 script. We have to guess which one to use.
if "\n" .. getline(1, 32)->join("\n") =~# '\n\s*vim9\%[script]\>'
- setlocal commentstring=#%s
+ setlocal commentstring=#\ %s
else
setlocal commentstring=\"%s
Sorry, I missed that comment. Thought I read through all of them
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Seems we only got approvals here. If you could do me one more favor please?
EDIT: Should I explicitly give space padding to commentstrings for files that use setl cms&? (Like C)
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!
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Riley Bruins
@ribru17 pushed 1 commit.
- f89f984 feat(runtime): add padding to commentstrings
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Clason
@clason commented on this pull request.
In runtime/ftplugin/xdefaults.vim:
> @@ -13,7 +14,7 @@ set cpo&vim let b:undo_ftplugin = "setl com< cms< inc< fo<" -setlocal comments=s1:/*,mb:*,ex:*/,:! commentstring& inc& +setlocal comments=s1:/*,mb:*,ex:*/,:! commentstring=/*\ %s\ */ inc&
I think the default here was actually incorrect; the correct commentstring is !\ %s, see tpope/vim-commentary#85
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
dkearns
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.
That is a good point. I tested it briefly with
fold.cfrom this repository. Vim seems to handle it gracefully, even so my'commentstring'was set to/*%s*/and I could successfully delete foldmarkers created with// %s. But still I think we shouldn't change the default 'commentstring' option.
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Riley Bruins
@ribru17 pushed 1 commit.
- ec2f0be feat(runtime): add padding to commentstrings
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
alright then. I'll fix this up while merging. Thanks all
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Clason
You forgot to include the fix to the xs commentstring (see last force-push).
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.