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

Conversation

@Shudrum
Copy link
Contributor

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 eslint bot added the triage label Aug 21, 2018

@platinumazure

This comment has been minimized.

Copy link
Member

commented Sep 15, 2018

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(

This comment has been minimized.

Copy link
@g-plane

g-plane Sep 23, 2018

Member

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

This comment has been minimized.

Copy link
@Shudrum

Shudrum Sep 25, 2018

Author Contributor

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

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

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

This comment has been minimized.

Copy link
Contributor Author

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 Shudrum force-pushed the Shudrum:camel-case-exceptions branch from 1321c96 to df1121a Sep 25, 2018

@Shudrum

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

PR updated.

Maybe I missed something, do not hesitate.

@aladdin-add aladdin-add added accepted and removed evaluating labels Sep 30, 2018

@ilyavolodin ilyavolodin merged commit 066f7e0 into eslint:master Oct 2, 2018

5 checks passed

commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

Thanks for the PR! Sorry for the long wait.

@Shudrum

This comment has been minimized.

Copy link
Contributor Author

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 Shudrum:camel-case-exceptions branch Oct 2, 2018

btmills added a commit that referenced this pull request Oct 12, 2018

@shuotongli

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

commented Oct 16, 2018

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

This comment has been minimized.

Copy link

commented Oct 16, 2018

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

commented Oct 16, 2018

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

@shuotongli

This comment has been minimized.

Copy link

commented Oct 16, 2018

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

This comment has been minimized.

Copy link

commented Oct 16, 2018

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

@shuotongli

This comment has been minimized.

Copy link

commented Oct 16, 2018

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

This comment has been minimized.

Copy link

commented Oct 16, 2018

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.
Projects
None yet
7 participants
You can’t perform that action at this time.