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

Update: camelcase rule ignoreList added #10783

Merged
merged 2 commits into from Oct 2, 2018
Merged

Update: camelcase rule ignoreList added #10783

merged 2 commits into from Oct 2, 2018

Conversation

Shudrum
Copy link
Contributor

@Shudrum Shudrum commented Aug 21, 2018

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[ ] New rule
[X] Changes an existing rule
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What rule do you want to change?

camelcase

Does this change cause the rule to produce more or fewer warnings?

No, only less if used.

How will the change be implemented? (New option, new default behavior, etc.)?

New option to the rule.

Please provide some example code that this change will affect:

class MyComponent extends React.Component {
  UNSAFE_componentWillMount() {
    // ...
  }
}

What does the rule currently do for this code?

It throw an error: Identifier 'UNSAFE_componentWillMount' is not in camel case.

What will the rule do after it's changed?

Using the added option: { ignoreList: ["UNSAFE_componentWillMount"] } will allow this method name.

What changes did you make? (Give an overview)

Camel case is used, more and more, but there is still many use of underscore, basically for:

  • The _id for MongoDB
  • So, many people still use underscore for identifiers, like: user_id
  • Also, some methods of some frameworks like React have methods like UNSAFE_componentWillMount
  • etc.

I have seen some discussions, it seems that people is trying to find the best way to do what this update add:

  • Ignoring all variables finishing with _id, with the option { ignoreList: ["_id$"] }
  • Ignoring all methods starting with UNSAFE_, with the option { ignoreList: ["^UNSAFE_"] }
  • Or add some of their specific variables: { ignoreList: ["_main_contex_"] }

Basically, it allow to ignore specific variables, and/or regexp pattern, simply.

One of the discussions, for example: #9435

Also: I've seen this PR: #10503, after my work :( But I prefer mine, do not hesitate to close this one. My bad.

Is there anything you'd like reviewers to focus on?

Tests, I have added some tests for my rule, but no overkill tests basically cover some already covered scenarios.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Aug 21, 2018
@aladdin-add aladdin-add added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Aug 22, 2018
@platinumazure
Copy link
Member

Hi @Shudrum, thanks for the PR. I apologize that we let this sit so long.

I've given #10503 a nudge. Hoping that the team can evaluate that one in the next few days. If that issue is accepted, we can review and potentially merge this PR.

Thanks for your patience!

* @private
*/
function isIgnored(name) {
return ignoreList.findIndex(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use !ignoreList.includes(/* */).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with includes is that I cannot execute a function to check values.

I need findIndex to check the ignoreList (soon renamed to allowedList) regular expressions.

@platinumazure
Copy link
Member

Hi @Shudrum, thanks for contributing and apologies for letting this sit so long.

It looks like a separate issue was created (#10503), where the ESLint team eventually accepted a slightly different proposal.

Would you be willing to make some changes to this PR to follow that proposal? Or, if you think the proposal we accepted does not cover the use cases you had envisioned in this PR, would you be willing to leave a comment and explain what might need to be changed?

If you're busy and no longer have time to work on this, that's okay too.

Thanks and sorry again for letting this slip through the cracks.

@Shudrum
Copy link
Contributor Author

Shudrum commented Sep 25, 2018

If I have understood:

The other PR allow a list of authorised variables:

"camelcase": ['error', {
  "exceptions": [
    "UNSAFE_componentDidMount",
    "UNSAFE_componentWillReceiveProps",
    "UNSAFE_componentWillUpdate"
  ]
}]

Mine do the same thing, but allow also regex:

"camelcase": ['error', {
  "ignoreList": [
    "UNSAFE_componentDidMount",
    "_id$",
    "^UNSAFE_"
  ]
}]

As asked by @sindresorhus: #10503 (comment)

I think this is the best way to manage this option.

I will update my PR from the review, also, I will change ignoreList by allow as asked by @nzakas: #10503 (comment)

@Shudrum
Copy link
Contributor Author

Shudrum commented Sep 25, 2018

PR updated.

Maybe I missed something, do not hesitate.

@aladdin-add aladdin-add added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 30, 2018
@ilyavolodin ilyavolodin merged commit 066f7e0 into eslint:master Oct 2, 2018
@ilyavolodin
Copy link
Member

Thanks for the PR! Sorry for the long wait.

@Shudrum
Copy link
Contributor Author

Shudrum commented Oct 2, 2018

I was the CTO / Main maintainer of PrestaShop during some time… I'll NEVER say anything about merging / delay answers.

It was really hard, slower, and I got way less Issues / PR as you.

Good job guys, and thank you!

@Shudrum Shudrum deleted the camel-case-exceptions branch October 2, 2018 12:12
btmills added a commit that referenced this pull request Oct 12, 2018
@shuotongli
Copy link

shuotongli commented Oct 16, 2018

I added 'camelcase': ['error', { allow: ['^UNSAFE_'] }], to the rules, but failed at linting:

Configuration for rule "camelcase" is invalid:
Value {"allow":["^UNSAFE_"]} should NOT have additional properties.

eslint -v is v5.7.0

@Brandon-Beck
Copy link

Just to be sure, are you using that version of eslint directly @shuotongli? If you are using it indirectly, such as via an editor, it is possible that the program is using its own version of eslint.
I am using the Atom editor, and the linter-eslint package for it has yet to be updated to this version. Had to tell it to use the system eslint to get this working. That error message matches what you would get from older versions.

@shuotongli
Copy link

Just to be sure, are you using that version of eslint directly @shuotongli? If you are using it indirectly, such as via an editor, it is possible that the program is using its own version of eslint.
I am using the Atom editor, and the linter-eslint package for it has yet to be updated to this version. Had to tell it to use the system eslint to get this working. That error message matches what you would get from older versions.

Thank you for your response @Brandon-Beck ! I'm running linting in the terminal while having this error msg. So I suppose I'm using it directly.

@Brandon-Beck
Copy link

Brandon-Beck commented Oct 16, 2018

Inside a terminal, navigate to your project's directory and then run
eslint -v to double check your version, and then
eslint ./ to attempt to check all js files.

If you still get the error, and the version still shows as v5.7.0, then you probably have a different problem that someone more knowledgeable here will need to address. If it works, then your sublime plugin may be set to use a different version of eslint (perhaps one local to the current project).

Depending what plugin your using, and how you configured it (if applicable), it may already be using the systems eslint.

@shuotongli
Copy link

shuotongli commented Oct 16, 2018

Inside a terminal, navigate to your project's directory and then run
eslint -v to double check your version, and then
eslint ./ to attempt to check all js files. If you still get the error, and the version still shows as v5.7.0, then you probably have a different problem. If it works, then your sublime plugin may be set to use a different version of eslint (perhaps one local to the current project).

Depending what plugin your using, it may already be using the systems eslint.

This is the terminal output:

11:34 $ eslint -v
bash: eslint: command not found
11:42 $ npx eslint -v
npx: installed 125 in 5.378s
v5.7.0
11:43 $ npx eslint ./
npx: installed 125 in 3.647s
No ESLint configuration found.

@Brandon-Beck
Copy link

Try again with npx eslint --config path/to/eslintrc ./

@shuotongli
Copy link

Try again with npx eslint --config path/to/eslintrc ./

11:52 $ npx eslint --config path/to/eslintrc ./
Cannot read config file: /Users/shuotong/web-components/path/to/eslintrc
Error: ENOENT: no such file or directory, open '/Users/shuotong/web-components/path/to/eslintrc'

@Brandon-Beck
Copy link

Just to be sure, you did replace that with your actual eslintrc path? Double check for spelling errors with ls or cat

@shuotongli
Copy link

Just to be sure, you did replace that with your actual eslintrc path? Double check for spelling errors with ls or cat

I couldn't find the eslintrc file in the package that I want to edit.

@Brandon-Beck
Copy link

It is probably hidden (.eslintrc.js, .eslintrc.json, .eslintrc.yaml, or perhaps even just plain .eslintrc), sorry, forgot to mention that. Try ls -a to see hidden files

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants