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

(question) eslint rule / plugin for guideline 7.15 #1731

Open
shelbyspeegle opened this issue Feb 13, 2018 · 11 comments
Open

(question) eslint rule / plugin for guideline 7.15 #1731

shelbyspeegle opened this issue Feb 13, 2018 · 11 comments

Comments

@shelbyspeegle
Copy link

is there an eslint rule or plugin that enforces 7.15?

@ljharb
Copy link
Collaborator

ljharb commented Feb 18, 2018

Indeed! comma-dangle handles the trailing comma, and function-paren-newline, i believe, handles the rest.

comma-dangle should be linked here already; however, I'm still not 100% sure about function-paren-newline :-)

@shelbyspeegle
Copy link
Author

awesome! thanks @ljharb.

i can't seem to find the right options for function-paren-newline that adheres to the airbnb eslint guideline 7.15.

in particular, i can't get eslint to bug me about the 'bad' example below.

// bad
console.log(foo,
  bar,
  baz);

// good
console.log(
  foo,
  bar,
  baz,
);
@ljharb
Copy link
Collaborator

ljharb commented Feb 22, 2018

Maybe we need to file something upstream on eslint about it?

@raunofreiberg
Copy link
Contributor

@shelbyspeegle I tried both rules out and I'm getting both errors, the bad example included.

"comma-dangle": "error",
"function-paren-newline": "error"

What have you tried so far?

@shelbyspeegle
Copy link
Author

shelbyspeegle commented Mar 6, 2018

thanks @raunofreiberg, that gets me close, unfortunately it allows these scenarios:

// bad?
console.log(
  foo,
  bar, baz,
  yo,
);

// bad?
console.log(
  foo, bar, baz,
  yo,
);

i assumed that this should produce an eslint error, but an example isnt provided for this type of case.

i did see this in the guide language that could suggest that this should error.

with each item on a line by itself

@ljharb
Copy link
Collaborator

ljharb commented Mar 7, 2018

Yes, that should be an error. Either the entire invocation fits on one line, or each argument is on a line by itself.

@GusBuonv
Copy link
Contributor

GusBuonv commented Sep 10, 2021

Doesn't the "multiline" option on function-paren-newline along with the current "comma-dangle" implement 7.15 correctly? If so, this line should be updated. Edit: I can open a PR, but I want to verify this is correct first. Also, I think it should be "multiline-arguments" not "multiline" so as to allow line breaks before and after argument lists with a single item.

@shelbyspeegle
Copy link
Author

@GusBuonv That works great for me!

For this test file, eslint errors show:
image

... and eslint --fix now properly resolves that to:
image

Using your suggested rules in this simple eslintrc.js.

module.exports = {
  extends: ['airbnb'],
  rules: {
    'function-paren-newline': ['error', 'multiline'],
  },
};

Thanks @GusBuonv!

@ljharb
Copy link
Collaborator

ljharb commented Sep 13, 2021

Reading the rule docs, it does seem like multiline-arguments is the option that would most closely match the guide's intent.

It was changed from "multiline" to "consistent" in 2018. eslint itself added multiline-arguments in 2019, so it seems like "consistent" was better than "multiline", but "multiline-arguments" may be better still.

I'm hesitant to make this change, however, until it's at least been verified on airbnb's own code. I'll reach out to folks there and see how it goes.

@ljharb
Copy link
Collaborator

ljharb commented Sep 13, 2021

Given that this rule is disabled by eslint-config-prettier, Airbnb won't be affected, so I think this is a reasonable semver-minor change to make. @GusBuonv please make that PR :-)

@GusBuonv
Copy link
Contributor

@ljharb Done! If the PR is merged, it may still be a good idea to keep this issue open or open a new one. I assume 7.15 is also supposed to apply to argument destructuring. If it is meant to apply in this case, it is still not properly enforced, and current core ESLint rules don't provide a way to achieve the desired behavior.

ljharb pushed a commit to GusBuonv/javascript that referenced this issue Nov 4, 2021
ljharb added a commit to GusBuonv/javascript that referenced this issue Nov 4, 2021
…`multiline-arguments` option

Related to airbnb#1731

Co-authored-by: Augustus Buonviri <augustus.buonviri@stratusgrid.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment