[vim/vim] Refactored zig ftplugin, removed upstream comment, and removed aucmd and auto formatting support. (PR #13803)
Mathias Lindgren
https://github.com/ziglang/zig.vim added auto formatting and linting (and population of the qflist) on save as a aucmd with an opt-out configuration, and was then merged into vim runtime. This seems to me like something that should be left up to that plugin and not be a part of vim.
It should probably be opt-in, as it is very unexpected behavior and uncharacteristic of vim to do something like this automatically, or the formatting (and error parsing stuff) should be removed entirely, as it should be left up to the user to register that kind of behavior if they want it through a plugin. This PR does the latter.
I could not find any concrete guidelines of what built in file type plugins should and should not do, I am personally fine with whatever, opt-in, or remove the format/astcheck functionality. I have however noted that the rust filetype support also has a lot of options available as opt-in, and it also always registers an aucmd.
Is this the direction vim is moving in? i.e. upstreaming oppinionated plugins for integrating languages tightly
My expectation before going down this rabbit hole was that built-in language supports only provided: filetype, filetype detection, syntax, indentation, and a makeprg.
The zig.vim plugin wants to be an ide-like plugin, and this PR removes the upstream dependency for the ftplugin implementation. I also made some consistency changes to the code in how undo_ftplugin is handled. The only significant thing left is the g:zig_std_dir and execution of zig env, which might also be up for discussion if it should be removed.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/13803
Commit Summary
- 394e87d Refactored zig ftplugin, removed upstream comment, and removed aucmd and auto formatting support.
File Changes
(2 files)
- D runtime/autoload/zig/fmt.vim (100)
- M runtime/ftplugin/zig.vim (45)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Maxim Kim
I believe autocmd that does autoformat should absolutely not happen by default.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Maxim Kim
I would also avoid all g:zig_std_dir set up:
" Safety check: don't execute zig from current directory
if !exists('g:zig_std_dir') && exists('*json_decode') &&
\ executable('zig') && dist#vim#IsSafeExecutable('zig', 'zig')
silent let s:env = system('zig env')
if v:shell_error == 0
let g:zig_std_dir = json_decode(s:env)['std_dir']
endif
unlet! s:env
endif
On windows gvim system() call looks ugly and you probably don't want to see cmd.exe window popup upon opening zig file.
Should be either opt-in or just a ftplugin help entry on how to set up g:zig_std_dir
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
dkearns
Is there a reason not to send your more general changes through the maintainer's repo?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Maxim Kim
Is there a reason not to send your more general changes through the maintainer's repo?
They might disagree with the proposed changes, but yeah it needs to be addressed there too(first?).
PS,
Do we have vim runtime files guide? What to avoid in ftplugin/syntax files?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
The general changes can be added to the zig.vim plugin after the fact, if they want them.
This is a proposal to split from that plugin where vim maintains its own ftplugin implementation (as zig.vim wants to do things that should probably not be in the vim runtime).
And please note that I am only referring to runtime/ftplugin/zig.vim not the syntax/indentation/compiler implementations, I see no problem with those, so they shold still be upstreamed.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
Just as an example, the go ft plugin was split into two, one bare bones and one more ide-like.
https://github.com/google/vim-ft-go
https://github.com/fatih/vim-go
The former one was merged into vim and is now obsolete. zig.vim is comparable to the vim-go plugin.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
> setlocal formatoptions-=t formatoptions+=croql - -setlocal suffixesadd=.zig,.zir +setlocal suffixesadd=.zig,.zir,.zon +let &l:define='\v(<fn>|<const>|<var>|^\s*\#\s*define)' +let b:undo_ftplugin = 'setl isk< fo< sua< mp< def<' + +if get(g:, 'zig_recommended_style', 1)
if get(g:, 'zig_recommended_style', 1)
That variable should be documented. Please place a short section into runtime/doc/syntax.txt (e.g. see how it is done for other synax files, like :h ft-python-syntax . Don't forget to add a tag and update the last change header please.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Pavel Zolotarevskiy
What is currently blocking this PR? Is it just the undocumented variable?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
yes
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Pavel Zolotarevskiy
@Tiseno could you please add the missing doc section? This is a blocking PR for neovim/neovim#28878
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Gregory Anders
Refactoring/rewriting the entire plugin means we can no longer pull changes from upstream (at least not easily). Would it not be simpler to just change the default, as in #14820?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Gregory Anders
Here is a patch for the docs:
diff --git a/runtime/doc/filetype.txt b/runtime/doc/filetype.txt index deb947cf1..feb946252 100644 --- a/runtime/doc/filetype.txt +++ b/runtime/doc/filetype.txt @@ -829,6 +829,12 @@ The mappings can be disabled with: > let g:no_vim_maps = 1 +ZIG *ft-zig-plugin* + + *g:zig_recommended_style* +The Zig |ftplugin| sets options according to Zig's recommended style. This can +be disabled by setting |g:zig_recommended_style| to 0. + ZIMBU *ft-zimbu-plugin* The Zimbu filetype plugin defines mappings to move to the start and end of
I put it in doc/filetype.txt rather than doc/syntax.txt since the variable is relevant for the ftplugin, not the syntax file.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Thanks, makes sense. I'll have a look later today
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
The doc comment!
But there is also the matter of the zig_std_dir, which I would like to remove because it also seems intrusive.
But I do not even know fully what its purpose is.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
@Tiseno pushed 1 commit.
- a5cd00b Added docs for ft-zig-plugin g:zig_recommended_style.
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Gregory Anders
But there is also the matter of the zig_std_dir, which I would like to remove because it also seems intrusive. But I do not even know fully what its purpose is.
It is used to set the 'path' variable, which is useful for e.g. gf, :find, [I, etc. If the ftplugin is setting 'include', 'includexpr', and 'define' already, then setting 'path' is probably a useful thing to do. And gating it behind the existence of g:zig_std_dir (so that users can just set it themselves) seems harmless enough.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
Ok, seems good to me, but is the implementation good enough?
@habamax pointed out
On windows gvim system() call looks ugly and you probably don't want to see cmd.exe window popup upon opening zig file.
Should be either opt-in or just a ftplugin help entry on how to set up g:zig_std_dir
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
@Tiseno commented on this pull request.
> setlocal formatoptions-=t formatoptions+=croql - -setlocal suffixesadd=.zig,.zir +setlocal suffixesadd=.zig,.zir,.zon +let &l:define='\v(<fn>|<const>|<var>|^\s*\#\s*define)' +let b:undo_ftplugin = 'setl isk< fo< sua< mp< def<' + +if get(g:, 'zig_recommended_style', 1)
What is meant by tag and last change header?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -1,44 +1,44 @@ " Vim filetype plugin file " Language: Zig -" Upstream: https://github.com/ziglang/zig.vim
So no more upstream?
> @@ -829,6 +829,13 @@ The mappings can be disabled with: > let g:no_vim_maps = 1 +ZIG *ft-zig-plugin* + + *g:zig_recommended_style* +The Zig |ftplugin| sets options according to Zig's recommended style. This can +be disabled by setting |g:zig_recommended_style| to 0.
Do we have a reference, what the Zig recommended style is?
> @@ -50,19 +50,10 @@ if !exists('g:zig_std_dir') && exists('*json_decode') &&
endif
if exists('g:zig_std_dir')
g:zig_std_dir should also be documented
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Gregory Anders
Ok, seems good to me, but is the implementation good enough? @habamax pointed out
On windows gvim system() call looks ugly and you probably don't want to see cmd.exe window popup upon opening zig file.
Should be either opt-in or just a ftplugin help entry on how to set up g:zig_std_dir
We don't need to shell out or call system(). If the user sets g:zig_std_dir themselves, the ftplugin can use it to set 'path'. Otherwise leave 'path' alone. But g:zig_std_dir probably needs to be included in the docs too.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
where is this system() call?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Gregory Anders
@gpanders commented on this pull request.
> @@ -1,44 +1,44 @@ " Vim filetype plugin file " Language: Zig -" Upstream: https://github.com/ziglang/zig.vim
The changes here differ significantly enough from upstream that it is not really a copy of the upstream plugin at all anymore. Perhaps we can leave a comment that the ftplugin is based on the upstream plugin though.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Gregory Anders
@gpanders commented on this pull request.
> @@ -829,6 +829,13 @@ The mappings can be disabled with: > let g:no_vim_maps = 1 +ZIG *ft-zig-plugin* + + *g:zig_recommended_style* +The Zig |ftplugin| sets options according to Zig's recommended style. This can +be disabled by setting |g:zig_recommended_style| to 0.
Can link to https://ziglang.org/documentation/0.12.0/#Style-Guide maybe? But that might link rot.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Gregory Anders
@chrisbra The original implementation (upstream) has a system() call (system('zig env')). The question was (I believe) whether this should be preserved or removed.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -1,44 +1,44 @@ " Vim filetype plugin file " Language: Zig -" Upstream: https://github.com/ziglang/zig.vim
Then please also add: Maintainer: This runtime file is looking for a new maintainer. (similar to other files):
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
@Tiseno commented on this pull request.
> @@ -829,6 +829,13 @@ The mappings can be disabled with: > let g:no_vim_maps = 1 +ZIG *ft-zig-plugin* + + *g:zig_recommended_style* +The Zig |ftplugin| sets options according to Zig's recommended style. This can +be disabled by setting |g:zig_recommended_style| to 0.
Yes https://ziglang.org/documentation/master/#Whitespace
Should that be referenced in the section?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
@Tiseno commented on this pull request.
> @@ -1,44 +1,44 @@ " Vim filetype plugin file " Language: Zig -" Upstream: https://github.com/ziglang/zig.vim
Yes, that was the idea. This is a split from the ziglang/zig.vim implementation of the ftplugin/zig.vim
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Ah, okay, let's leave it out for now. I am not a big fan of running system commands just by loading a filetype. Just document that g:zig_std_dir can be set by system('zig env') in a custom filetype auto command
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Gregory Anders
@gpanders commented on this pull request.
> @@ -1,44 +1,44 @@ " Vim filetype plugin file " Language: Zig -" Upstream: https://github.com/ziglang/zig.vim
I am happy to act as maintainer (unless @Tiseno would like to). I upstreamed the original runtime files.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Gregory Anders
Just document that
g:zig_std_dircan be set bysystem('zig env')in a custom filetype auto command
Specifically, the std_dir field from the output of zig env. If we want a one-liner, it's :echo json_decode(system('zig env'))['std_dir'].
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
@Tiseno commented on this pull request.
> @@ -1,44 +1,44 @@ " Vim filetype plugin file " Language: Zig -" Upstream: https://github.com/ziglang/zig.vim
I can become maintainer, that would be cool!
Something like
" Maintainer: Mathias Lindgren <math.l...@gmail.com>
" Based on: https://github.com/ziglang/zig.vim
then?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
@Tiseno pushed 1 commit.
- 37244cd Removed auto detection of zig std dir and documented g:zig_std_dir.
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -1,44 +1,44 @@ " Vim filetype plugin file " Language: Zig -" Upstream: https://github.com/ziglang/zig.vim
yes exactly like this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -829,6 +829,13 @@ The mappings can be disabled with: > let g:no_vim_maps = 1 +ZIG *ft-zig-plugin* + + *g:zig_recommended_style* +The Zig |ftplugin| sets options according to Zig's recommended style. This can +be disabled by setting |g:zig_recommended_style| to 0.
Either like this, or have a look how it is done for python, explicitly mentioned what properties are set :h ft-python-plugin
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
> setlocal formatoptions-=t formatoptions+=croql - -setlocal suffixesadd=.zig,.zir +setlocal suffixesadd=.zig,.zir,.zon +let &l:define='\v(<fn>|<const>|<var>|^\s*\#\s*define)' +let b:undo_ftplugin = 'setl isk< fo< sua< mp< def<' + +if get(g:, 'zig_recommended_style', 1)
Basically add a help jump target, like it is done now: *g:zig_recommended_style* in the help file. And then we usually have a separate line, mentioning when the last change was done, like here: https://github.com/vim/vim/blob/master/runtime/ftplugin/c.vim#L4
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
@Tiseno pushed 2 commits.
- d7cbf82 Reworded zig filetype documentation.
- e0d12a5 Added myself as maintainer for ftplugin/zig.vim
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
@Tiseno pushed 1 commit.
- 8147346 Added last change header.
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
Ah, okay, let's leave it out for now. I am not a big fan of running system commands just by loading a filetype. Just document that g:zig_std_dir can be set by system('zig env') in a custom filetype auto command
Now I just declared that one can put it in a aucmd, but do we want a full example for that instead?
How would one actually do that in the most idiomatic way?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Gregory Anders
@gpanders commented on this pull request.
> +recommended style (https://ziglang.org/documentation/master/): + + `setlocal expandtab shiftwidth=4 softtabstop=4 tabstop=8` +
For code, use >, not backticks (which is Markdown syntax).
-recommended style (https://ziglang.org/documentation/master/): - - `setlocal expandtab shiftwidth=4 softtabstop=4 tabstop=8` - +recommended style (https://ziglang.org/documentation/master/): > + + setlocal expandtab shiftwidth=4 softtabstop=4 tabstop=8 +<
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Gregory Anders
@gpanders commented on this pull request.
> +recommended style (https://ziglang.org/documentation/master/): + + `setlocal expandtab shiftwidth=4 softtabstop=4 tabstop=8` +
Same for the line below.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Gregory Anders
> +The path to the zig standard library. The Zig |ftplugin| reads g:zig_std_dir +and appends it to the |path| for zig files. Where the zig standard library +is located is system and installation method dependant.⬇️ Suggested change
-The path to the zig standard library. The Zig |ftplugin| reads g:zig_std_dir -and appends it to the |path| for zig files. Where the zig standard library -is located is system and installation method dependant. +The path to the Zig standard library. The Zig |ftplugin| reads g:zig_std_dir +and appends it to 'path' for Zig files. Where the Zig standard library +is located is system and installation method dependent.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Gregory Anders
> +One can automatically read the std_dir using `zig env`:
+ `let g:zig_std_dir = json_decode(system('zig env'))['std_dir']`
+
+This can, for example, be put in a FileType |autocmd| to only load when a zig
+file is opened.
⬇️ Suggested change
-One can automatically read the std_dir using `zig env`:
- `let g:zig_std_dir = json_decode(system('zig env'))['std_dir']`
-
-This can, for example, be put in a FileType |autocmd| to only load when a zig
-file is opened.
+One can automatically set |g:zig_std_dir| using `zig env`: >
+ let g:zig_std_dir = json_decode(system('zig env'))['std_dir']
+<
+This can, for example, be put in a FileType |autocmd| or user |ftplugin| to only load when a Zig
+file is opened.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
@Tiseno pushed 1 commit.
- 9634efa Fixed code formatting.
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
@Tiseno pushed 1 commit.
- eb30ce7 Fixed code formatting of zig filetype documentation.
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
@Tiseno pushed 1 commit.
- 63bc3a6 Added highlight to g:zig_recommended_style and g:zig_std_dir in two more places.
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
@Tiseno commented on this pull request.
> @@ -50,19 +50,10 @@ if !exists('g:zig_std_dir') && exists('*json_decode') &&
endif
if exists('g:zig_std_dir')
Fixed
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Mathias Lindgren
@Tiseno commented on this pull request.
> @@ -829,6 +829,13 @@ The mappings can be disabled with: > let g:no_vim_maps = 1 +ZIG *ft-zig-plugin* + + *g:zig_recommended_style* +The Zig |ftplugin| sets options according to Zig's recommended style. This can +be disabled by setting |g:zig_recommended_style| to 0.
Fixed, I added both the link and the set fields
—
Reply to this email directly, view it on GitHub, or unsubscribe.
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 Brabandt
thanks all!
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.