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
add handling of lone commas when destructuring arrays #12772
add handling of lone commas when destructuring arrays #12772
Conversation
48a941e
to
2d555c4
Compare
Does the same lone commas problem exist with array expressions as well? If so, maybe we shouldn't separate logic for ArrayPattern/ArrayExpression, but rather fix both. |
The issue is there for those, yes. However, I cannot think of any good use case for anyone to willingly create an array with empty positions.
For destructuring assigments it should always be allowed (hence this fix)... but maybe users would want or not to enforce the same thing for array expressions. Perhaps that should be handled as a new option, and contributed in a different PR, rather than assume people will want it off-the-bat (breaking the current, expected functioning of ESLint.) |
I agree there is probably no valid use case for empty positions in arrays, but there's a separate rule to disallow them (
If this is a bug in the default behavior for It's good in general to fix separate bugs in separate PRs, but in this particular case it looks like we are adding some extra code just to preserve one bug while fixing another? |
That's a good point, as long as there's something to ensure discouraging the use of empty positions RHS, we're good. I'm just not well acquainted with the whole array of rules (bad pun.) Amending my PR. Stand by. That rule should also be amended to ensure it allows destructuring assignments, perhaps? |
2d555c4
to
01cb061
Compare
}); | ||
if (arrayLiteral) { | ||
|
||
// ignored element, move on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Place holder in case we may want to report other things later, based on new options.
01cb061
to
deb43bd
Compare
@mdjermanovic It seems allowing those RHS empty positions makes a few of the tests fail. Since this PR is meant to relax the "comma-style" rule, as you suggest, correctly delegating the handling of those empty spaces to "no-sparse-arrays"; I'll remove the tests here, and make a mental note to check/adjust there instead. |
deb43bd
to
832f906
Compare
To clarify,
Failing tests means that we changed something in the designed behavior of this rule. We should analyze what's failing, is that correct, and then modify those tests and/or add new tests to show the new behavior. This isn't only about array expressions, the same change would affect similar tests with array patterns. It's just a coincidence that only array expressions' tests had empty elements. |
{ | ||
code: "var foo = [\n(bar\n)\n,\nbaz\n];", | ||
output: "var foo = [\n(bar\n),\nbaz\n];", | ||
errors: [{ | ||
messageId: "unexpectedLineBeforeAndAfterComma", | ||
type: "Identifier" | ||
}] | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular test doesn't have empty elements. If it's failing, then the change might be incorrect since it modifies behavior for arrays/patterns that don't have empty elements.
A simplified example, this time with an array pattern that doesn't have empty elements:
/*eslint comma-style: ["error", "last", {
"exceptions": { "ArrayPattern": false }
}]*/
var [
foo
,
bar] = [];
This is invalid code by this rule in the actual version - Online Demo
After this change as it's implemented at the moment, this would be a valid code (I think, it would be nice to add a test case). Is this the desired new behavior, i.e., should we ignore any commas on separate lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the desired new behavior, i.e., should we ignore any commas on separate lines?
If "comma-style" is enabled, that comma in a new line belongs either with foo
(as default 'last'), or bar
(if set to 'first'). Meaning your example should indeed fail.
My attempted fix is therefor incomplete.
Will revise and amend it... thanks for pointing it out. 👍
FYI: Still working on this. Just slammed at the 9-5 for the moment. |
Thanks for the update :) |
Unfortunately, it looks like this pull request has been abandoned, so closing. Note: The issue that this PR addresses is accepted (#12756) so anyone interested may submit another PR to implement the change. |
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Details about the bug and my environment are as seen in the issue
Closes #12756. This handles ignored elements as per MDN
Tell us about your environment
What changes did you make? (Give an overview)
I've split the checks for ArrayPatterns from ArrayLiterals, as LHS allows ignoring elements, whereas RHS would be invalid code. This results in null nodes using AST, which the existing validation code could not handle coherently. Thus, if there are any commas without element, it doesn't report them. Honestly, I'm not sure my approach is the most adequate, yet it seems to get the job done based on my tests.
I've noticed with this change you can have as many commas in a single line as you want.
I am not sure what space between consecutive commas should be required, if any.
That is to be discussed in a separate issue.
For the time being, my next step will be modifying "array-element-newline" to respond accordingly (pending issue to discuss implementation with the community, will add a link here for reference.)
Is there anything you'd like reviewers to focus on?
I didn't go the whole ten yards checking for other overlapping rules.