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
fix!: correct camelcase
rule schema for allow
option
#18232
Conversation
Hi @eMerzh!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Good catch! Current schema actually allows arrays with multiple elements, as in this test case: eslint/tests/lib/rules/camelcase.js Lines 328 to 331 in dadc5bf
But it only validates that the first element is a string, while the others can be anything, as in this example: /* eslint camelcase: ["error", { allow: ["a_b", 5, "c_d"] }] */ // ok configuration
let a_b; // ok
let c_d; // ok
let e_f; // error here |
camelcase
rule schema for allow
option
Per our semver policies, this is a breaking change (Part of the public API is removed or changed in an incompatible way > Rule schemas). @eslint/eslint-tsc Since we've already released v9 rc, looks like we'll have to leave this for ESLint v10? |
@mdjermanovic i understand your position but if i understand, this shouldn't break your CI as normally you could "only" have "one string" in the array... thus this technically allows you do to more Also it's only in the validation side, not on the lint itself |
The current schema already allows multiple strings in the array (demo). After this change, a configuration like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will fix eslint-types/eslint-define-config#278 semi-automatically (after the generate rule-types script is executed)
This looks like a bug that should be fixed. I'm noting that the current logic allows arbitrary elements in the /*eslint camelcase: ["error", {allow: ["foo", {toString: null}]}]*/
function bar_baz() {
} We could look into how to improve option validation for this rule in a non-breaking way, but that wouldn't be the end of the job, so probably it's best to schedule the proper fix for v10 if it can't be done earlier. |
What i also noticed that if the |
@mdjermanovic If we are categorizing this as a bug fix, then I'm okay with including in the next RC. RCs (and betas) really are designed to catch bugs. As long as we're not adding anything new, I think it's appropriate to include in v9. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to include it in v9, but we need:
- add a few tests.
- add it in the migration guide: https://github.com/eslint/eslint/blob/main/docs/src/use/migrate-to-9.0.0.md
Sounds good to me, marking this as accepted. |
i was thinking about adding those in tests/lib/rules/camelcase.js but it is really the place? 🤔 also i was thinking of a test with 2-3 allows and 1 or 2 with numbers or objects in options, did you think about something else? |
RuleTester currently doesn't support testing invalid options. (it will when we implement https://github.com/eslint/rfcs/tree/main/designs/2023-test-rule-errors). I think it would be fine to just add a couple more tests with valid options ( |
3e192fe
to
d56af9e
Compare
tests/lib/rules/camelcase.js
Outdated
{ | ||
code: "var$key = 0;", | ||
options: [{ allow: ["__option_foo__", "\\$key"] }] | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code would be valid without options, maybe you intended to use some other variable name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! Would like another review before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! Since this is a breaking change, it requires another approval from @eslint/eslint-tsc before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
The Json schema of the camelCase rule validate that the first element is a string instead of validating that it's an array of string
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Correct the Json Schema
Is there anything you'd like reviewers to focus on?
Nope