-
Notifications
You must be signed in to change notification settings - Fork 137
fix!: options in .editorconfig not work with shfmt #1165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
15729f3
to
ea99523
Compare
I recommend to use the .editorconfig to set the shfmt instead of global config in nvim. It is simple and has many benefits.
Related issue: #1161
Must pass --filename option to shfmt. When shfmt read content from stdin, it doesn't know the filename and its extension. So it won't match the patterns "[*.sh]" and "[*.bash]" in .editorconfig. Do not pass any Parser and Printer options like -i/-p/-bn/-l. It will cause the .editorconfig not to be loaded. See https://github.com/mvdan/sh/blob/23633a432f903599a4ce46c30c4337e413a26ef1/cmd/shfmt/main.go#L186-L196 Breaking Change: Removed shfmtConfig options. Use the .editorconfig options instead of. The .editorconfig options of shfmt refer to https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd#examples
ea99523
to
86e7671
Compare
This change would require users to create an .editorconfig
in order to configure shfmt
. I think a better approach would be to use .editorconfig
to set the formatting options if present, whilst still supporting configuration via the editor. It already does this for indentation if your editor is set up correctly, but will need special handling for the other options. I did consider this as part of the original implementation, but didn't want to overcomplicate things too early! If there's demand then I'm happy to look into this.
The simplest approach is to create a ~/.editorconfig
for global configuration for editor, and .editorconfig
under project root for project specifics.
Current options of shfmtConfig are incomplete that lacking of "keep_padding", "shell_variant", "indent_style" and "indent_size". And when shfmt changed its options, bash-language-server has to change and release new version. That is overcomplicate.
And the .editorconfig file can be store in git to share with co-workers. But options in editor configuration like vim codes are unshareable.
This wasn't included initially as it has been deprecated: mvdan/sh#658 However, it has been mentioned in bash-lsp#1165 and is a valid option in the current version of `shfmt`, so support has been added (with a suitable deprecation warning).
The simplest approach is to create a
~/.editorconfig
for global configuration for editor, and.editorconfig
under project root for project specifics.
That's fine as long as people are happy doing that, but not all users will want to - some will want to configure this via their editor as they do for other options.
Current options of shfmtConfig are incomplete that lacking of "keep_padding", "shell_variant", "indent_style" and "indent_size".
keep_padding
was left out as it was deprecated, but my latest PR adds this in as it has been mentioned more than once.shell_variant
is still not supported via editor configuration asshfmt
's detection is pretty good. Again, if there's demand then it can be added.- Indentation configuration is not specific to
shfmt
, so you configure this as usual in your editor (or via.editorconfig
) andshfmt
will use that. My latest PR clarifies this in the README.
And when shfmt changed its options, bash-language-server has to change and release new version. That is overcomplicate.
Fair point. But they don't change frequently so I don't think that's a good reason not to give users the option to configure this via their editor.
And the .editorconfig file can be store in git to share with co-workers. But options in editor configuration like vim codes are unshareable.
Yes, I understand the benefits of .editorconfig
and use this myself for some projects, but not everyone wants to do that. I believe in giving users a choice, so think both ways should be supported. I'll raise another PR soon enough to add support for .editorconfig
in addition to editor config. :-p
@chris-reeves Thanks for the detailed explanation. I agree with giving users more choices.
@Shane-XB-Qian How to set these options of shfmt in vim modeline?
The project specific vimrc seems to be a good idea. But it only works for oneself but not co-workers. Because the project specific vimrc may have conflicts with co-workers' global vimrc.
The shfmt options in .editorconfig still not work on version 5.3.4. The key point at:
Do not pass any Parser and Printer options like -i/-p/-bn/-l. It will cause the .editorconfig not to be loaded. See mvdan/sh@23633a4/cmd/shfmt/main.go#L186-L196
If we decide to support shfmt options both in .editorconfig and in editor config, it will get complicated.
It is necessary to look up .editorconfig file recursively, and parse the .editorconfig content in bash-lsp. Then the merged the options from editorconfig and shfmtConfig would be passed to shfmt.
The shfmt options in .editorconfig must have higher weight than it in shfmtConfig. Otherwise, the options in .editorconfig would never work because the shfmtConfig always override them.
If only check the .editorconfig file existed and don't parse and merge the .editorconfig options, it is not enough. User may write a .editorconfig for markdown/lua/js files but not for shellscripts.
The bash-lsp must know the shfmt options in .editorconfig for current file to decide whether use shfmtConfig as default or not.
I think it is a bad design that parsing .editorconfig twice in both bash-lsp and shfmt.
What's your advice? @chris-reeves
You're right that we're going to have to be a wee bit clever about this. As you say, just looking for an .editorconfig
in the path isn't going to cut it as it may not apply to shell scripts. Likewise we can't blindly apply .editorconfig
if there is shell script config in there, because it might only specify indentation config with no shfmt
options. My plan is to read and parse the .editorconfig
config for the file, combine it with the existing language server config (with a configurable priority) and then pass those options to shfmt
. So .editorconfig
will only be parsed once (by the language server, not shfmt
).
It sounds quite complicated, but should be fairly straightforward to implement. I have this sketched out in my head and should have something by the weekend, assuming I get some time on this over the next couple of evenings.
@chris-reeves It looks good to me. Thanks.
Uh oh!
There was an error while loading. Please reload this page.
When shfmt read content from stdin, it doesn't know the filename and its extension. So it won't match the patterns "[.sh]" and "[.bash]" in .editorconfig.
For example, my .editorconfig file in project root.
You can compare the results between
cat ./your-script.sh | shfmt --filename=your-script.sh -d -
andcat ./your-script.sh | shfmt -d -
.Breaking Change:
Removed shfmtConfig options. Use the .editorconfig options instead of. The .editorconfig options of shfmt refer to https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd#examples