[[FEAT]] Add new "enforcing" option: trailingcomma (updated)
#3090
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Missing trailing commas in array and object literals can result in
undetectable visual ambiguities--the same visual ambiguities that lead
to bugs when a programmer relies on automatic semicolon insertion.
Therefore, it might be a good practice to always use them, so this
option provides warnings in their absence.
For example, this code might have worked last Tuesday:
[
b + c
].forEach(print);
But if one adds an element to the array and forgets to compensate for
the missing comma, no syntax error is thrown, and a linter cannot
determine if this was a mistake or an intentional function invocation:
[
b + c
(d + e)
].forEach(print);
If one always appends a list item with a comma, this ambiguity cannot
occur:
[
b + c,
].forEach(print);
[
b + c,
(d + e),
].forEach(print);
If this option is `true`, a warning will be thrown if a "," does not
occur before the closing "]" or "}" in an array or object literal. If
option `es3` is also `true`, this option does nothing, because otherwise
the options would contradict each other, and the `es3` mandate is more
pressing.
There is no inverse of this option, that would warn if an ES5 programmer
omitted a trailing comma. Omission of a trailing comma in ES5 provides
no benefit, unlike option `trailingcomma`, which actually helps prevent
bugs. Such an inverse feature would be considered "stylistic;" do not
confuse `trailingcomma` as a "style" option, as it is no more
"stylistic" than the default detection of semicolons is.
Resolving conflicts: - The `inES5` method is now defined on the state object directly; update call sites accordingly - Remove `true` parameter from invocation of `inES5` so that ES6+ environments may also opt in to this functionality - Update the identifier of the warning to accomodate warnings that have been introduced since the patch was submitted - Enforce this restriction even in the absence of the configuration option for array initializers in ES3
| @@ -2743,6 +2743,9 @@ var JSHINT = (function() { | |||
| break; | |||
| } | |||
| } else { | |||
| if (state.option.trailingcomma && state.inES5()) { | |||
There was a problem hiding this comment.
Do you think the ES5 check should be removed? (Continuing from here: #2100 (comment).)
From a language purist perspective, it should be. Although for practical reasons it is probably still useful for any sorry soul who still has to support old IE. I don't see an option specifically for detecting old IE bugs, otherwise I'd suggest that instead.
There was a problem hiding this comment.
JSHint has a few IE-specific warnings (e.g. W070, "Extra comma. (it breaks
older versions of IE)"), and rather than referencing an option dedicated to
that condition, it applies these a little inconsistently--when the environment
does not support ES5. So this behavior respects precedence.
But like you, I'm not sure the precedent is a good one. I'm thinking that a new
"enforcing" option like oldie might be good, especially from a language
purist's perspective. I've written up a proposal here: gh-3094
| /** | ||
| * This option warns when a comma is not placed after the last element in an | ||
| * array or object literal. Due to bugs in old versions of IE, trailing | ||
| * commas used to be discouraged, but since ES5 their semantics were |
|
This is great work :) |
jugglinmike
added a commit
to jugglinmike/jshint
that referenced
this pull request
Apr 9, 2017
Missing trailing commas in array and object literals can result in
undetectable visual ambiguities--the same visual ambiguities that lead
to bugs when a programmer relies on automatic semicolon insertion.
Therefore, it might be a good practice to always use them, so this
option provides warnings in their absence.
For example, this code might have worked last Tuesday:
[
b + c
].forEach(print);
But if one adds an element to the array and forgets to compensate for
the missing comma, no syntax error is thrown, and a linter cannot
determine if this was a mistake or an intentional function invocation:
[
b + c
(d + e)
].forEach(print);
If one always appends a list item with a comma, this ambiguity cannot
occur:
[
b + c,
].forEach(print);
[
b + c,
(d + e),
].forEach(print);
If this option is `true`, a warning will be thrown if a "," does not
occur before the closing "]" or "}" in an array or object literal. If
option `es3` is also `true`, this option does nothing, because otherwise
the options would contradict each other, and the `es3` mandate is more
pressing.
There is no inverse of this option, that would warn if an ES5 programmer
omitted a trailing comma. Omission of a trailing comma in ES5 provides
no benefit, unlike option `trailingcomma`, which actually helps prevent
bugs. Such an inverse feature would be considered "stylistic;" do not
confuse `trailingcomma` as a "style" option, as it is no more
"stylistic" than the default detection of semicolons is.
This was referenced Oct 26, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changeset continues the work first submitted by @jacksonrayhamilton in
gh-2100. As discussed in that pull request, a few changes to
masterin theyears since he submitted have introduced conflicts in the patch. I've resolved
those with a merge commit because I believe that's the most transparent way to
document this collaboration.