Skip to content
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

cmd/shfmt: add config file #234

Closed
nkakouros opened this issue May 9, 2018 · 5 comments
Closed

cmd/shfmt: add config file #234

nkakouros opened this issue May 9, 2018 · 5 comments

Comments

@nkakouros
Copy link

It would be great if there were a config file from where shfmt would read the user configuration.

This would have the benefit of allowing easy sharing/enforcing of the formatting options of a project.

It would also make it easier to add more configuration options for things that are currently hard coded (if I am not mistaken). For instance, comments at the end of a command line are reformatted to have always one space between the end of the command and the # sign. Coming from, eg python's pep8, one might want to have 2 spaces. Having a variable that could be set in the config file seems to me a simple way to add more customizability to shfmt.

@mvdan
Copy link
Owner

mvdan commented May 9, 2018

I'm not convinced that a config file would bring any real benefit. Could one not simply write a shell script calling the tool with the appropriate flags?

You also mention extra flags/options for the tool, but that seems like a separate discussion. Even if a config file was added, that wouldn't affect the number of options available to modify the tool's behavior.

Also, you might want to have a look at the -kp flag, which can be used to keep custom vertical alignments and other spacings that you might want. That should let you control how you want comments to be spaced, without having to add another flag.

@mvdan mvdan changed the title Add config file cmd/shfmt: add config file May 9, 2018
@nkakouros
Copy link
Author

nkakouros commented May 9, 2018

My train of thought was this. In the future it is probable that more options get added to the tool. Lets say that at a point there are 10 available formatting options. In an existing project, a specific formatting is chosen that is translated into a command like:

shfmt -i -bn -ci -kp -w -p -q 5 -r -z9 -bd -hi -mi

There is no context in this and it is difficult to understand what each option does without having the help output side by side.

Apart from that, it seems to me clearer to say to a new contributor to a project to run shfmt --config shmft.conf instead of the above or to configure their editor according to the config file instead of the line above.

This might be weak reasons, I agree. Anyway, having this thought in mind, I stumbled upon the "issue" with the inline comments. I thought I should open a feature request to have a cli option. But what if there is more functionality that one might want to have an option for? Of course, it gets ugly and perhaps complicated to have a cli option for every desired configuration point. So, instead, if a config file was available, it could accommodate a lot more options. The file could be parsed into variables and instead of saying in the code eg:

p.spaces(p.commentPadding + 1)
say sth like:
p.spaces(p.commentPadding + inlineCommentSpacing)

(I am not even sure this is the relevant line of code, just used it to give an example).

Anyway, this was my thinking. Should this be closed and a feature request on the inline comment spacing be opened? On that topic, the -kp option is not sufficient for this use case. Saying 'keep the spaces I have as they are' is different to 'make sure there are exactly 2 spaces before the inline comment'.

@mvdan
Copy link
Owner

mvdan commented May 10, 2018

I see what you mean about it being easier to have more options with a config file. With or without a config file though, I'd like to keep the number of options to a minimum. They make the tool more complex to understand and use, but especially to develop.

Most of them aren't much added code; the issue is rather how they all play together. For example, how should -mn affect the other printing flags? Should -kp override -i and -ci? Even with the small amount of flags we have now, there are already tricky combinations of flags that may behave unexpectedly, or even break the tool since noone had tested those weird combinations before.

I realise that the flag you're asking for is on the simple side, but even then, it would still add complexity and confusion with others like -kp. Deciding whether or not to add more flags isn't just about the code :) Once added, I am stuck with a feature/option practically forever.

I hope that answers the "if a config file was available, it could accommodate a lot more options" point. I simply think that's not true, even if I had the man-hours to maintain and fix all these combined edge cases in the future. So I'm fairly sure that we don't want a config file, given that I doubt that many more parsing/printing flags will be added.

On that topic, the -kp option is not sufficient for this use case. Saying 'keep the spaces I have as they are' is different to 'make sure there are exactly 2 spaces before the inline comment'.

Yes, that is true. A simple tool can't accomodate all use cases :) When -kp was used, it also wasn't exactly what the original user was requesting. They were asking that shfmt automatically padded code as follows:

foo bar    etc
foo baaaar etc

As you can imagine though, enforcing all vertical alignments would be a humongous task, and the tool would likely get it wrong more often than not. So I ended up with an option that was simpler and more generic, and which would let the user control these aspects of the code format.

In a way, the same applies to your request. Which is why I was wondering whether the flag would be good enough for your use case.

@mvdan
Copy link
Owner

mvdan commented Jun 2, 2018

Since this issue is primarily about adding a config file, and I'm pretty certain it's not a good idea (see all my points above), I'm going to close this issue for now.

If you want to suggest adding extra options and flags, please file separate issues. Each new knob needs to be proven useful though, as they all bring their downsides.

@mvdan
Copy link
Owner

mvdan commented Apr 26, 2020

If anyone reads this thread in the future, bear in mind that we implemented EditorConfig support instead - see #393 and the docs in the README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants