koalaman / shellcheck

ShellCheck, a static analysis tool for shell scripts
https://www.shellcheck.net
GNU General Public License v3.0
36.38k stars 1.78k forks source link

Enhancement: human-readable code names for errors #1197

Open kapsh opened 6 years ago

kapsh commented 6 years ago

AFAIU currently the only one way to identify shellcheck errors are their numbers. My suggestion is to add human-readable and human-understandable names too.

Rationale:

  1. While reading the code, you can understand what exactly is disabled here.
  2. You can pick the code name from shellcheck output to write directive which will comply with the 1.
  3. You can quickly scan shellcheck output for code names instead of reading long messages (less important, but it helps too).

For new checks and feature suggestions

Here's a snippet or screenshot that shows the problem:


#!/your/interpreter
# shellcheck disable=SC2034
a=( foo=bar )

Here's what shellcheck currently says:

^-- SC2191: The = here is literal. To assign by index, use ( [index]=value ) with no spaces. To keep as literal, quote it.

Here's what I wanted or expected to see:


#!/your/interpreter
# shellcheck disable=unused-variable
a=( foo=bar) 

And as a result:

^-- SC2191 (literal-equal): The = here is literal. To assign by index, use ( [index]=value ) with no spaces. To keep as literal, quote it.
koalaman commented 6 years ago

I'm not sure about the logistics of managing a second set of IDs, but I definitely see how this would help the disable lines.

Dmole commented 6 years ago

I don't really see added value to this suggestion because nothing is stopping you from adding descriptive comments to your disable directives. As shellcheck grows the names would approach the length of the descriptions in order to remain unique.

matthewpersico commented 6 years ago

@Dmole - Yeah, your're right. Just ran down the list of SC codes trying to conjure up a human-readable strings for the id codes. Trying to make useful short ones is not an easy task.

Perhaps I should try to get flycheck in emacs to popup the text of the id in the disable when I put the mouse or cursor on the id.

rwe commented 5 years ago

This doesn't need to be an all-or-nothing thing; for warnings that defy brief description, numeric codes can always be used first. (Or, human codes could cover multiple numeric codes; some codes are very similar). But I think the insistence on only numeric codes is one of shellcheck's primary friction points.

In the presence of suppressions, new readers aren't going to see the message at all. Which means there's an extra burden on writers to make redundant comments describing what their disable=SC2016 suppression referring to, or an extra burden on readers who have to stop reading and go do try to find a reference, if they care; and hopefully they normally care.

An example of a system that does this reasonably well is pylint, which supports both a good number of numeric codes as well as helpful human-readable IDs.

tomasohara commented 2 years ago

I was working on a pylint-inspired suggestion, before I found this issue.

If there is a way to disable using unique keywords from the message, then the mapping can be derived automatically. For example, SC2034 could be specified in a few different ways:

         # shellcheck disable=appears-unused-verify
-or-
         # shellcheck disable=appears-unused

Shellcheck would need to provide mapping from various keyword combinations to the error code. (It can require consecutive words and omit function words to avoid too many mappings.) The burden would be on the user to use a unique combination.

marvingreenberg commented 1 year ago

I think this is desirable - many other linters take this approach. Spellcheck is even internally inconsistent. Certain actions support this like directives:

enable=require-double-brackets (I'm not sure if you can say enable=SC2292)

As @rwe mentioned, it doesn't have to be all or nothing. If certain rules are hard to abbreviate (or really are unlikely to be disabled?) (no-unicode-sequence-for-single-quote, bad-shebang-leading-space, no-ls-piped-to-grep ??) then could only support a code.

Yes you can add documentation, but if I mistype a global disable=SCNNN (comment "do something innocuous") and generally it makes mistakes much easier and harder to review.

But, it is a non-trivial undertaking, so maybe someone (me?) should make a PR to consider the issue more concretely.