"array-element-newline" must handle ignored elements when destructuring arrays #12773
Comments
justincorrigible
added
bug
|
This looks like two separate issues to me. The first is a bug fix: I think the expected behavior would be for the rule to treat each comma as if it were an element in the array: /*eslint array-element-newline: ["error", "always"]*/
// Correct
const [
configPath = 'config.json',
,
authFilePath = 'freshbooks.auth',
,
,
] = process.argv;/*eslint array-element-newline: ["error", "never"]*/
// Correct
const [
configPath = 'config.json', authFilePath = 'freshbooks.auth', , ,
] = process.argv;This is not the current behavior, and I would advocate for fixing it in a semver-minor bug fix PR. It doesn't look like null destructured array elements are in tests either, so it doesn't seem like it has been considered up to this point. Secondly, I think it makes sense to update the rule with an option that allows for groups of commas to be either ignored or treated as a single entity. |
kaicataldo
added
accepted
tl;dr: Agreed. That's what I've meant. Fair, I've simply thought since I was going to tackle the bug, might well add the extra config if others agreed with that option being desirable. (I don't really care, but some may.) If the topic grows and there's interest in debating it, I'll branch it out in a new ticket. Else, I can just open a separate PR for the option, and tag both to this issue. Let's see how it goes.
Not quite sure what you mean by this, and yes, I've noticed there are no tests for destructuring assignments neither here, nor at "comma-style". We can proceed as you see fit, but it is my first couple attempted contributions at ESLint, so thanks for bearing with me. |
|
We appreciate your help!
What I meant by this is that, while this will result in more errors being reported, that I think we should treat it as a bug fix and therefore releasable in a minor release (indicating non-breaking changes). You can read more about our semantic versioning policy here (and happy to explain if anything is unclear). That being said, we've begun work on our next major release and are currently able to make breaking changes. Let's see what others on the team have to say! |
What would be the correct formatting if we also have /*eslint array-element-newline: ["error", "always"]*/
/*eslint comma-style: ["error", "first", {
"exceptions": { "ArrayPattern": false }
}]*/
// Correct ?
const [
configPath = 'config.json'
,
,authFilePath = 'freshbooks.auth'
,
,
,
] = process.argv;I'm not sure if this is the expected code, especially about those 3 commas at the end, but perhaps we should analyze could there be any conflicts and how will the fixers for these two rules together transform the code into the desired state (this, or some other), e.g., which one will put the linebreak between Currently, /*eslint array-element-newline: ["error", "always"]*/
/*eslint comma-style: ["error", "first", {
"exceptions": { "ArrayPattern": false }
}]*/
const [
configPath = 'config.json',
,authFilePath = 'freshbooks.auth',
,
,] = process.argv;The new /*eslint array-element-newline: ["error", "always"]*/
/*eslint comma-style: ["error", "first", {
"exceptions": { "ArrayPattern": false }
}]*/
const [
configPath = 'config.json',
,
authFilePath = 'freshbooks.auth',
,
,] = process.argv;And there's a conflict, |
Haven't been able to sit down and test some options out (slammed at my 9-5 for the moment), but it just hit me: Those three commas are redundant... the point of ignored elements is that you care about another one after. Those dangling 3 from the example are being ignored for no reason, and I think they should be flagged by "comma-style". In regards to @mdjermanovic, I agree with all of his response. |
I agree they are most likely a mistake (though, those 2 empty elements at the end of the pattern are still invoking
|
|
Agreed, yet wouldn't "comma-style" normally flag a dangling comma, if that option is disabled? |
Yes, it could have been an option in the Dangling commas are required/disallowed by the It was a design decision to separate these preferences into two rules, though a single rule to enforce both things would also make sense.
It's different because 1 dangling comma has no semantics. Code has the same behavior with or without that comma. One dangling comma or not is just a "layout" preference. On the other hand, multiple dangling commas in array expressions and array patterns have a meaning. They represent elisions, which do change the behavior of the code: array expression will create an array with more elements, array pattern will pull more elements from the source. It's probably best practice to avoid this feature, but this isn't something that "layout" rules such as |
|
Indeed. I agree wholeheartedly. Just had to play devil’s advocate to document the decisions. Will work on this once done with #12772 |
|
2020年1月11日(土) 6:49 Anders Richardsson <notifications@github.com>:
… *Tell us about your environment*
- *ESLint Version:* 6.8.0
- *Node Version:* 13.6.0
- *NPM Version:* 6.13.4
- *Parser* @typescript-eslint <https://github.com/typescript-eslint>
*Please show your full configuration:*
Configuration ```js module.exports = { env: { browser: true, node: true,
es6: true, }, extends: [ 'airbnb', 'plugin:react/recommended',
'plugin:import/typescript', ], globals: { PropTypes: false, React: false,
uiVersion: false, }, parser: ***@***.***/parser', parserOptions: {
ecmaFeatures: { jsx: true, }, ecmaVersion: 2018, sourceType: 'module', },
plugins: [ ***@***.*** <https://github.com/typescript-eslint>',
'react', 'react-hooks', 'sort-destructure-keys', ], rules: {
'array-bracket-newline': ['error', { 'multiline': true, 'minItems': 3, }],
'array-element-newline': ['warn', { 'multiline': true, 'minItems': 3, }],
'arrow-body-style': 'off', 'arrow-parens': 'off', 'camelcase': ['warn', {
allow: ["^UNSAFE_", "doc_count", "^active_"], }], 'comma-dangle': ['warn',
'always-multiline'], 'consistent-return': ['warn', {
treatUndefinedAsUnspecified: true }], 'func-names': ['warn', 'as-needed'],
'function-paren-newline': ['warn', 'consistent'],
'implicit-arrow-linebreak': 'off', 'indent': ['warn', 4, { ArrayExpression:
'first', CallExpression: { arguments: 'first' }, flatTernaryExpressions:
true, FunctionDeclaration: { parameters: 'first' }, FunctionExpression: {
parameters: 'first' }, ignoreComments: true, ignoredNodes: [
'ConditionalExpression', 'JSXAttribute', 'JSXClosingElement', 'JSXElement',
'JSXElement > *', 'JSXEmptyExpression', 'JSXExpressionContainer',
'JSXIdentifier', 'JSXMemberExpression', 'JSXNamespacedName',
'JSXOpeningElement', 'JSXSpreadAttribute', 'JSXSpreadChild', 'JSXText',
'TemplateLiteral > *', ], ImportDeclaration: 'first', MemberExpression: 1,
ObjectExpression: 'first', SwitchCase: 1, VariableDeclarator: 'first', }],
'multiline-ternary': ['warn', 'always-multiline'], 'no-console': ['warn', {
allow: [ 'info', 'warn', 'error', ] }], 'no-debugger': 'warn',
'no-fallthrough': ['warn', { commentPattern: 'break[\\s\\w]*omitted', }],
'no-nested-ternary': 'off', 'no-unneeded-ternary': 'warn',
'no-unused-expressions': ['warn', { allowShortCircuit: true,
allowTaggedTemplates: true, allowTernary: true, }], 'no-var': 'error', //
Must use const or let. 'object-property-newline': ['warn', { //
allowAllPropertiesOnSameLine: false, }], 'object-curly-newline': 'warn', //
['warn', { // ObjectExpression: { // 'multiline': true, // 'minProperties':
2, // }, // ObjectPattern: { // 'multiline': true, // 'minProperties': 2,
// }, // ImportDeclaration: { // 'multiline': true, // 'minProperties': 2,
// }, // ExportDeclaration: { // 'multiline': true, // 'minProperties': 2,
// }, // }], 'operator-linebreak': ['warn', 'after', { overrides: { '?':
'before', ':': 'before', } }], 'padded-blocks': 'error', 'semi': ['warn',
'always'], 'sort-keys': ['warn', 'asc', { caseSensitive: false, natural:
true, }], 'quotes': ['warn', 'single'], 'import/extensions': 'off',
'react/jsx-closing-bracket-location': ['warn', 'props-aligned'],
'react/jsx-filename-extension': ['warn', { extensions: [ '.js', '.jsx',
'.tsx', ], }], 'react/jsx-first-prop-new-line': ['warn', 'multiline'],
'react/jsx-fragments': ['error', 'element'], 'react/jsx-indent': ['warn',
2, { checkAttributes: true, indentLogicalExpressions: true, }],
'react/jsx-indent-props': ['warn', 2], 'react/jsx-max-props-per-line':
['warn', { maximum: 1, when: 'multiline', }],
'react/jsx-one-expression-per-line': ['warn', { allow: 'single-child', }],
'react/jsx-sort-default-props': 'error', 'react/jsx-sort-props': ['warn', {
ignoreCase: true, }], 'react/jsx-tag-spacing': ['warn', { closingSlash:
'never', beforeSelfClosing: 'always', afterOpening: 'never', beforeClosing:
'never', }], 'react/jsx-wrap-multilines': ['error', { declaration:
'parens-new-line', assignment: 'parens-new-line', return:
'parens-new-line', arrow: 'parens-new-line', condition: 'parens-new-line',
logical: 'parens-new-line', prop: 'parens-new-line', }],
'react/no-did-mount-set-state': 'warn', 'react/no-did-update-set-state':
'warn', 'react/no-direct-mutation-state': 'warn', 'react/no-multi-comp':
'warn', 'react/no-unknown-property': 'warn', 'react/sort-comp': 'warn',
'react/sort-prop-types': 'error', 'react/prop-types': 'off',
'sort-destructure-keys/sort-destructure-keys': ['warn', { caseSensitive:
false, }], ***@***.***/explicit-function-return-type': 'off',
***@***.***/explicit-member-accessibility': 'off', }, settings: {
'import/resolver': { node: { paths: ['./src/packages'] }, }, react: {
version: 'detect', }, }, } ```
*What did you do? Please include the actual source code causing the issue,
as well as the command that you used to run ESLint.*
As continuation of the fix for #12756
<#12756>, I expect each element
(ignored or not) in a new line. However, while working on that other issue,
I've realised different users may prefer having consecutive ignored
elements (i.e. consecutive commas) in the same line.
Examples of different cases:
const [
,, // consecutive commas. valid?
configPath = 'config.json',
authFilePath = 'freshbooks.auth',
] = process.argv;
// vs
const [
,
, // enforce separate lines?
configPath = 'config.json',
authFilePath = 'freshbooks.auth',
] = process.argv;
My only right now opinion is we should not allow consecutive commas after
a valid (as of before this "fix") line.
Example:
const [
configPath = 'config.json', // good old comma
, // this comma is now valid as an ignored element.
authFilePath = 'freshbooks.auth',,, // these two extra commas should be in the next line,
// either together or lines of their own.
] = process.argv;
This issue is meant to open a community discussion on what the handling
for successive commas should be; whether it should be a config in this rule
or not.
Afterwards, I'll try to implement the agreed changes, unless someone else
wants to take a stab at it.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#12773?email_source=notifications&email_token=AAOZDT2Q4ZTSGFGKQ3WHQKDQ5DUM3A5CNFSM4KFNW5JKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IFOQBJA>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOZDTYZ3PH6QBEONPMYTT3Q5DUM3ANCNFSM4KFNW5JA>
.
|
|
Unfortunately, it looks like there wasn't enough interest from the team Thanks for contributing to ESLint and we appreciate your understanding. |
|
Since the issue was auto closed, does this mean this won't be added? |
eslint-deprecated
bot
added
the
archived due to age
Tell us about your environment
Please show your full configuration:
Configuration
```js module.exports = { env: { browser: true, node: true, es6: true, }, extends: [ 'airbnb', 'plugin:react/recommended', 'plugin:import/typescript', ], globals: { PropTypes: false, React: false, uiVersion: false, }, parser: '@typescript-eslint/parser', parserOptions: { ecmaFeatures: { jsx: true, }, ecmaVersion: 2018, sourceType: 'module', }, plugins: [ '@typescript-eslint', 'react', 'react-hooks', 'sort-destructure-keys', ], rules: { 'array-bracket-newline': ['error', { 'multiline': true, 'minItems': 3, }], 'array-element-newline': ['warn', { 'multiline': true, 'minItems': 3, }], 'arrow-body-style': 'off', 'arrow-parens': 'off', 'camelcase': ['warn', { allow: ["^UNSAFE_", "doc_count", "^active_"], }], 'comma-dangle': ['warn', 'always-multiline'], 'consistent-return': ['warn', { treatUndefinedAsUnspecified: true }], 'func-names': ['warn', 'as-needed'], 'function-paren-newline': ['warn', 'consistent'], 'implicit-arrow-linebreak': 'off', 'indent': ['warn', 4, { ArrayExpression: 'first', CallExpression: { arguments: 'first' }, flatTernaryExpressions: true, FunctionDeclaration: { parameters: 'first' }, FunctionExpression: { parameters: 'first' }, ignoreComments: true, ignoredNodes: [ 'ConditionalExpression', 'JSXAttribute', 'JSXClosingElement', 'JSXElement', 'JSXElement > *', 'JSXEmptyExpression', 'JSXExpressionContainer', 'JSXIdentifier', 'JSXMemberExpression', 'JSXNamespacedName', 'JSXOpeningElement', 'JSXSpreadAttribute', 'JSXSpreadChild', 'JSXText', 'TemplateLiteral > *', ], ImportDeclaration: 'first', MemberExpression: 1, ObjectExpression: 'first', SwitchCase: 1, VariableDeclarator: 'first', }], 'multiline-ternary': ['warn', 'always-multiline'], 'no-console': ['warn', { allow: [ 'info', 'warn', 'error', ] }], 'no-debugger': 'warn', 'no-fallthrough': ['warn', { commentPattern: 'break[\\s\\w]*omitted', }], 'no-nested-ternary': 'off', 'no-unneeded-ternary': 'warn', 'no-unused-expressions': ['warn', { allowShortCircuit: true, allowTaggedTemplates: true, allowTernary: true, }], 'no-var': 'error', // Must use const or let. 'object-property-newline': ['warn', { // allowAllPropertiesOnSameLine: false, }], 'object-curly-newline': 'warn', // ['warn', { // ObjectExpression: { // 'multiline': true, // 'minProperties': 2, // }, // ObjectPattern: { // 'multiline': true, // 'minProperties': 2, // }, // ImportDeclaration: { // 'multiline': true, // 'minProperties': 2, // }, // ExportDeclaration: { // 'multiline': true, // 'minProperties': 2, // }, // }], 'operator-linebreak': ['warn', 'after', { overrides: { '?': 'before', ':': 'before', } }], 'padded-blocks': 'error', 'semi': ['warn', 'always'], 'sort-keys': ['warn', 'asc', { caseSensitive: false, natural: true, }], 'quotes': ['warn', 'single'], 'import/extensions': 'off', 'react/jsx-closing-bracket-location': ['warn', 'props-aligned'], 'react/jsx-filename-extension': ['warn', { extensions: [ '.js', '.jsx', '.tsx', ], }], 'react/jsx-first-prop-new-line': ['warn', 'multiline'], 'react/jsx-fragments': ['error', 'element'], 'react/jsx-indent': ['warn', 2, { checkAttributes: true, indentLogicalExpressions: true, }], 'react/jsx-indent-props': ['warn', 2], 'react/jsx-max-props-per-line': ['warn', { maximum: 1, when: 'multiline', }], 'react/jsx-one-expression-per-line': ['warn', { allow: 'single-child', }], 'react/jsx-sort-default-props': 'error', 'react/jsx-sort-props': ['warn', { ignoreCase: true, }], 'react/jsx-tag-spacing': ['warn', { closingSlash: 'never', beforeSelfClosing: 'always', afterOpening: 'never', beforeClosing: 'never', }], 'react/jsx-wrap-multilines': ['error', { declaration: 'parens-new-line', assignment: 'parens-new-line', return: 'parens-new-line', arrow: 'parens-new-line', condition: 'parens-new-line', logical: 'parens-new-line', prop: 'parens-new-line', }], 'react/no-did-mount-set-state': 'warn', 'react/no-did-update-set-state': 'warn', 'react/no-direct-mutation-state': 'warn', 'react/no-multi-comp': 'warn', 'react/no-unknown-property': 'warn', 'react/sort-comp': 'warn', 'react/sort-prop-types': 'error', 'react/prop-types': 'off', 'sort-destructure-keys/sort-destructure-keys': ['warn', { caseSensitive: false, }], '@typescript-eslint/explicit-function-return-type': 'off', '@typescript-eslint/explicit-member-accessibility': 'off', }, settings: { 'import/resolver': { node: { paths: ['./src/packages'] }, }, react: { version: 'detect', }, }, } ```What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.
As continuation of the fix for #12756, I expect each element (ignored or not) in a new line. However, while working on that other issue, I've realised different users may prefer having consecutive ignored elements (i.e. consecutive commas) in the same line.
Examples of different cases:
My only right now opinion is we should not allow consecutive commas after a valid (as of before this "fix") line.
Example:
This issue is meant to open a community discussion on what the handling for successive commas should be; whether it should be a config in this rule or not.
Afterwards, I'll try to implement the agreed changes, unless someone else wants to take a stab at it.
The text was updated successfully, but these errors were encountered: