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

curly multi-or-nest incorrect example with a comment #12972

Closed
mdjermanovic opened this issue Feb 28, 2020 · 5 comments
Closed

curly multi-or-nest incorrect example with a comment #12972

mdjermanovic opened this issue Feb 28, 2020 · 5 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules

Comments

@mdjermanovic
Copy link
Member

Tell us about your environment

  • ESLint Version: v7.0.0-alpha.1
  • Node Version: v12.14.0
  • npm Version: v6.13.4

What parser (default, Babel-ESLint, etc.) are you using?

Please show your full configuration:

Configuration
module.exports = {
    parserOptions: {
        ecmaVersion: 2015,
    },
};

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

Online Demo (v6.8.0)

/*eslint curly: ["error", "multi-or-nest"]*/

if (foo)
    // some comment
    bar();
eslint index.js

What did you expect to happen?

In curly multi-or-nest documentation this is one of the examples of incorrect code (the last one).

What actually happened? Please include the actual, raw output from ESLint.

no errors.

Are you willing to submit a pull request to fix this bug?

I'm not sure should this be fixed in the rule or in the docs. This may be a bug, but I have concerns about the impact if we change the behavior now.

Relevant issue/PRs: #7538, #7539, #7597.

The intention was to allow this:

/*eslint curly: ["error", "multi-or-nest"]*/

if (foo) {
    // some comment
    bar();
}

but I can't tell from the relevant discussions whether the intention was also to disallow the code that was added to the incorrect examples.

@mdjermanovic mdjermanovic added bug ESLint is working incorrectly rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Feb 28, 2020
@kaicataldo
Copy link
Member

kaicataldo commented Feb 28, 2020

Nice find! From the documentation:

You can use another configuration that forces brace-less if, else if, else, for, while, or do if their body contains only one single-line statement.

Given this heuristic, I don't think this should warn. That being said, it looks like the reason this isn't warning is because it ignores cases where there are comments (which I'm not sure is expected behavior). See demo here.

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Mar 31, 2020
@eslint-deprecated
Copy link

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 issues failing to reach accepted status after 21 days tend to
never be accepted, 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.

@kaicataldo
Copy link
Member

I would advocate for updating the documentation to match the behavior of the rule right now and to follow up with a separate PR if we decide that the behavior is incorrect.

@mdjermanovic
Copy link
Member Author

PR #13151 updates the docs to match the actual behavior.

I'd vote to leave the rule as is. This could be unintended behavior, but fixing it looks like a big change for users now.

@eslint-deprecated
Copy link

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 issues failing to reach accepted status after 21 days tend to
never be accepted, 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.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Nov 5, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

2 participants