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

"comma-style" doesn't like ignored, multiline destructured array elements. #12756

Closed
caravinci opened this issue Jan 7, 2020 · 13 comments
Closed
Assignees
Projects

Comments

@caravinci
Copy link

@caravinci caravinci commented Jan 7, 2020

Tell us about your environment

  • ESLint Version: 6.8.0
  • Node Version: 13.6.0
  • NPM Version: 6.13.4
  • Parser @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: '@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.

Tried destructuring an array, while ignoring the first two elements (as per MDN).
Expecting each element in a new line (my preference to ease code version management, discussion for another day), somehow the second comma is not happy unless the whole destructuring assignment is in a single line.

const [
    ,,
    configPath = 'config.json',
    authFilePath = 'freshbooks.auth',
] = process.argv;
const [
    ,
    ,
    configPath = 'config.json',
    authFilePath = 'freshbooks.auth',
] = process.argv;

In both cases I get a Bad line breaking before and after ','. (comma-style) error message for the second comma.

Thought I'd use the "ArrayPattern" exception, but that would turn linting off for all array destructuring, and that's less desirable. Seems to me like the rule should understand ignored elements (bug), or we should have another exception/handling for these consecutive commas.
Running ESLint automatically, using linter-eslint on Atom.

Thoughts?

While looking for other issues like this, and found a couple closed ones:

  • #1587 adds a fix, yet doesn't address multiline code.
  • #9968 closed without resolution, doesn't deal with multiline either.

Are you willing to submit a pull request to fix this bug?
Likely yes, if someone points me in the right direction (inside the repo).

@caravinci caravinci changed the title "comma-style" doesn't like ignored destructured array elements. "comma-style" doesn't like ignored, multiline destructured array elements. Jan 8, 2020
@fregante
Copy link
Contributor

@fregante fregante commented Jan 8, 2020

My suggestion for this would be to use .slice() if you're skipping more than one element, your intention is clearer and it's easier to count:

const [
    configPath = 'config.json',
    authFilePath = 'freshbooks.auth',
] = process.argv.slice(2);

@caravinci
Copy link
Author

@caravinci caravinci commented Jan 8, 2020

Clever workaround. Guess we can use that while the bug in the rule is resolved.
Thanks @fregante!

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jan 9, 2020

To scope this issue a bit more, this can be recreated with just one ignored element:

const [
    ,
    configPath = 'config.json',
    authFilePath = 'freshbooks.auth',
] = process.argv;

Additionally, here's the relevant configuration the original example is extending off of.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jan 9, 2020

I agree that this seems like a bug. Seems like we want to add some special logic for ArrayPatterns (code can be found here) that accounts for lines that contain ignored destructured array elements. I imagine we want to essentially ignore any lines that are only made of commas. It looks like we'll have to do it at the token level because empty destructured elements are represented as null in the AST.

@caravinci
Copy link
Author

@caravinci caravinci commented Jan 10, 2020

Progress update: "Comma-style" respects destructuring in my local.
Just in case anyone is interested in taking it for a spin, this is the commit

Now I'm trying to figure out the consequential response by "array-element-newline" to ensure the following:

// this shouldn't pass
const [
    ,
    foo,,,
    bar,
] = Array;


// this is ok
const [
    ,
    foo,
    ,,
    bar,
] = Array;

Should I perhaps make the changes to that rule a different issue, where to discuss with others what the right response/expectation should be?

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jan 10, 2020

Should I perhaps make the changes to that rule a different issue, where to discuss with others what the right response/expectation should be?

Rules are independent of each other, and since this relaxes the rule, I don't think we need to group the two issues together. Feel free to make a new issue!

@caravinci
Copy link
Author

@caravinci caravinci commented Jan 10, 2020

In that case I'll open a PR with my progress, and have that new issue be an incremental of this. Thanks, @kaicataldo

@eslint-deprecated eslint-deprecated bot closed this Apr 8, 2020
@eslint-deprecated
Copy link

@eslint-deprecated eslint-deprecated bot commented Apr 8, 2020

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@ljharb
Copy link
Contributor

@ljharb ljharb commented Apr 8, 2020

This is a bug; please reopen.

@eslint-deprecated
Copy link

@eslint-deprecated eslint-deprecated bot commented May 10, 2020

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@ljharb
Copy link
Contributor

@ljharb ljharb commented May 10, 2020

This remains a bug, please reopen.

@kaicataldo kaicataldo self-assigned this May 11, 2020
@kaicataldo kaicataldo reopened this May 11, 2020
@nzakas nzakas added this to Needs Triage in Triage via automation Mar 25, 2021
@nzakas nzakas moved this from Needs Triage to Ready to Implement in Triage Mar 25, 2021
@nzakas nzakas moved this from Ready to Implement to Pull Request Opened in Triage May 29, 2021
@boutahlilsoufiane
Copy link
Contributor

@boutahlilsoufiane boutahlilsoufiane commented Jul 13, 2021

I'm going to do this.

@boutahlilsoufiane
Copy link
Contributor

@boutahlilsoufiane boutahlilsoufiane commented Jul 26, 2021

Hi, the PR is created. I will appreciate anyone who can review it.

boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this issue Jul 29, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this issue Jul 29, 2021
@btmills btmills closed this in d7dc07a Jul 30, 2021
Triage automation moved this from Pull Request Opened to Complete Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Triage
Complete
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants