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

Also parse the color option from the config file #2350

Open
oliv3r opened this issue Oct 7, 2021 · 8 comments
Open

Also parse the color option from the config file #2350

oliv3r opened this issue Oct 7, 2021 · 8 comments

Comments

@oliv3r
Copy link

oliv3r commented Oct 7, 2021

Shellcheck seems to ignore the color= option if it is placed in the .shellcheckrc file. IMO any commandline argument should be also parseable from the config file (the user can still override this after all).

This feature request is to support honoring the color= directive from the config file.

oliv3r added a commit to oliv3r/shellcheck that referenced this issue Oct 7, 2021
@kurahaupo
Copy link

I suggest that the example for .shellcheckrc should use color=auto rather than color=always.

@koalaman
Copy link
Owner

koalaman commented Oct 9, 2021

As mentioned on the PR, you can have multiple .shellcheckrc files, one per directory, and they're applied to the appropriate files. How would this work when two rc files have conflicting formatting options?

@kurahaupo
Copy link

Ideally "nearest wins", so whichever first mentions it among a list something like this:

  • command line options
  • .shellcheckrc
  • ../.shellcheckrc
  • ../../.shellcheckrc
  • ../../../.shellcheckrc ... etc, relative to each file being inspected; then
  • $HOME/.shellcheckrc (traditional)
  • ${XDG_CONFIG_HOME:-$HOME/.config}/shellcheck/rc (modern)
  • /etc/shellcheck/rc.d/*
  • /etc/shellcheck/rc
  • /usr/share/shellcheck/rc.d/*
  • /usr/share/shellcheck/rc
  • hard-coded defaults

In a functional language I'm guessing that it's easier to go "use the current value if any, or look for the next one; but in a procedural language it might be easier to implement by reversing this list and having "last wins" simply by implicitly overwriting earlier values.

I wonder whether things like .../.git/hooks/shellcheck/rc should also be included.

@koalaman
Copy link
Owner

For the most part, this is how it already works for individual script files. Is the suggestion to look for the nearest .shellcheckrc relative to the current working dir when it comes to formatting options, and then look at the nearest .shellcheckrc relative to the script file for everything else? (The two can of course be the same)

@kurahaupo
Copy link

No, I don't think prioritizing the current directory would be as useful as relative to the file to be inspected.

My main intent is if there's precedence between the files, then it's not ambiguous, just pick a winner.

oliv3r added a commit to oliv3r/shellcheck that referenced this issue Oct 14, 2021
@oliv3r
Copy link
Author

oliv3r commented Oct 14, 2021

I agree, lets have the discussion here, and keep the MR technical.

To sort of repeat what I wrote there as well; I think that shellcheck should favor the local file for specific options, and for more 'global in kind' option whatever is in the root. I think generally for the per repo config files, its all relatively easy to figure out.

How those play with local config options, that's harder. E.g. if I have in my shellcheckrc in my homedir 'color=alwaysand the project explicitly put it tocolor=never(for whatever reason) should shellcheck favor my own setting? I would argue yes, just like using--color=always` would take precedence, right?

@kurahaupo
Copy link

kurahaupo commented Oct 16, 2021

I can envisage having Shellcheck as a pre-commit check in a repo, and in that case the project should control which things are treated as errors. For this purpose it's important that the output be parsable, so I would want the per-repo or per-directory options to take precedence, for all options (not just color=xxxx).

Whilst I can see how how having $HOME/.whatever take precedence for cosmetic adjustments seems useful, I think there will be conflicting opinions on where the line should be drawn between "cosmetic" and "API impacting".

I think most would agree that the difference between color=none and color=auto is merely a cosmetic user preference, but the difference between either of those and color=always is a significant API change that alters the behaviour of scripts that invoke shellcheck. I would also argue that ignore=SCnnnn changes a trigger from an error to a non-error and therefore is also an API change; but I can see that reasonable people might disagree with me.

Having different precedence order for different configuration options would be insanely complex, both for coding, and for human understanding. I would rather a consistent precedence with a simple logic: the further away a config file is from the file being inspected, the lower its precedence.

That said, I don't think having ignore options in $HOME/.shellcheckrc should allow you to make commits in a repo that wouldn't otherwise permit them. That would point towards treating all ignore options in a file as a unit for the purposes of precedence: if any ignore options are given in a "nearby" config file, then all ignore options in "more distant" config files are ignored. Unfortunately I'm not so clear on how that should interact with command line --ignore= options, and maybe there would need to be an additional config option to allow config files with lower precedence to add to the "ignore" list.

@koalaman
Copy link
Owner

I guess one possible approach is to get "global" options from the .shellcheckrc that applies to the first file given. If nothing else, it's very easy to understand.

That said, I don't think having ignore options in $HOME/.shellcheckrc should allow you to make commits in a repo that wouldn't otherwise permit them. That would point towards treating all ignore options in a file as a unit for the purposes of precedence

This is indeed the exact rationale for the current behavior.

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

3 participants