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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update: Allow regex in no-param-reassign ignore option array #11275

Conversation

lbennett-stacki
Copy link
Contributor

@lbennett-stacki lbennett-stacki commented Jan 16, 2019

Clickbait title. Not actually regex. Just a string that looks like a regex.

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

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

What rule do you want to change?

no-param-reassign: https://eslint.org/docs/rules/no-param-reassign

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

No difference.

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

Change how an existing option is used.

Please provide some example code that this change will affect:

function foo(fooBar) {
  fooBar.aaa++;
}

What does the rule currently do for this code?

Disallow Reassignment of Function Parameters, using strings

What will the rule do after it's changed?

Disallow Reassignment of Function Parameters, using strings and regex-like strings

What changes did you make? (Give an overview)

https://eslint.org/docs/rules/no-param-reassign

I didn't really like matching string literals for the ignorePropertyModificationsFor configuration array for no-param-reassign. If you want to avoid adding disable comments for this rule, you have to agree on a set of property names. These will have to be generic names that could be more descriptive and useful from a glance, or the array will continuously have more useful property names added to it and it will continue to grow.

Now we can agree on a naming pattern, rather than an explicit name. i.e. /^mutable.*$/. Now instead of just element, I can have mutableSettingsButton.

Understanding the code:

  • Parse all ignorePropertyModificationsFor array string items into RegExp objects.

    • If this item is a string surrounded in forward slashes (/), it will be parsed into /$1/$2, which will be used to instantiate a RegExp object with new RegExp($1, $2).
    • If this item is not surrounded in forward slashes, it will be parsed using new RegExp('^$1$') so it is backwards compatible.
  • Adds tests for valid and invalid regex cases.

  • Adds to a documentation example to demonstrate using a regex string item.

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

Personally, I couldn't decide the best way to approach this as my first eslint contribution. I personally would rather have separate and explicit rule configuration properties for regular expressions and string literals, but this seemed simpler than finding out what to call that configuration property. 馃槄I hope it's OK! Thanks a bunch for your time and efforts reviewing this 鉂わ笍

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jan 16, 2019
@lbennett-stacki lbennett-stacki force-pushed the regex-ignore-property-modifications-for branch from 42962cd to 359c3bd Compare January 16, 2019 05:34
@kaicataldo
Copy link
Member

@LukeeeeBennett So sorry this fell through the cracks. I'll refrain from reviewing the implementation until consensus has been reached, but this seems like a reasonable enhancement to me, especially since it's backwards compatible.

@kaicataldo kaicataldo 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 Feb 7, 2019
@platinumazure
Copy link
Member

I'm 馃憤 as long as it's a new property. This would otherwise be a breaking change. (Open to being persuaded otherwise but that's where I stand on this right now.)

@g-plane
Copy link
Member

g-plane commented Feb 8, 2019

It may not be a breaking change, because an identifier doesn't contain the character /.

@platinumazure
Copy link
Member

Apologies, I misread this as saying the term would just be thrown into the RegExp constructor:

If this item is not surrounded in forward slashes, it will be parsed using new RegExp('^$1$') so it is backwards compatible.

I understand this is a backwards compatible change.

I'm slightly concerned at the approach to using forward slashes as delimiters, not because it's a bad idea in this case (it's a fine idea), but because most/all of our other rules that accept regular expressions do so without requiring forward slash delimiters. I'm worried about inconsistency and potential user confusion. (That said, I actually wish we would require delimiters in all of those rules as it's a lot more clear, given RegExp literals are not themselves available in json and yaml configuration formats.)

@lbennett-stacki
Copy link
Contributor Author

lbennett-stacki commented Feb 13, 2019

@platinumazure I was considering a new configuration property initially; I have no strong opinion either way so if you'd rather lean on consistency let's do it that way, I'm more than happy to make the change. I see the pattern you talk about in this rule https://eslint.org/docs/rules/id-match.

@platinumazure
Copy link
Member

@LukeeeeBennett I would love to see us make a broader change around supporting slash-delimited strings as regex across the board, so we have consistency. Until then, I think we should just create a new option. Would you mind updating the original issue post with a new option proposal? Thanks so much! (Also, please accept my apologies for the delayed reply.)

@lbennett-stacki lbennett-stacki force-pushed the regex-ignore-property-modifications-for branch 2 times, most recently from 6575df5 to 6b1004d Compare May 7, 2019 04:29
Uses new `ignoredPropertyAssignmentsRegex` option.
@lbennett-stacki lbennett-stacki force-pushed the regex-ignore-property-modifications-for branch from 6b1004d to e798ea9 Compare May 7, 2019 04:32
@lbennett-stacki
Copy link
Contributor Author

lbennett-stacki commented May 7, 2019

(Also, please accept my apologies for the delayed reply.)

@platinumazure Hah, not a problem! Here is a much more delayed response.

We now have an additional option and this is ready for review.

@lbennett-stacki
Copy link
Contributor Author

Feel free to let me know if anything is required for this to be merged. :)

@platinumazure
Copy link
Member

We need a champion (from the team) and one more 馃憤 to accept the issue. That said, I'm still thinking we might need to have a broader discussion on handling user-supplied "regex" input.

@eslint/eslint-team Anyone want to champion this issue?

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 26, 2019

I'll champion this, and there are already 3 馃憤 , so marking as accepted.

A design question. The current proposal is an additional array of regexes. There is a similar PR #12305 for another rule, with the proposal to add a flag that would turn the existing array into an array of regexes.

Both proposals are avoiding a breaking change, but it would be good to have consistency, so the question is - is an additional array preferred over an additional flag, as a way to avoid breaking changes?

Edit: Disregard this post, this proposal is already accepted in its current form. Also, the other PR is updated to be consistent with this one.

@mdjermanovic mdjermanovic self-assigned this Sep 26, 2019
@mdjermanovic mdjermanovic 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 26, 2019
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM in general! Left some notes about the docs and an additional test.

It would be also nice to update the commit message and PR title with the name of the new option.

function foo(bar) {
bar_.aaa++;
}
```
Copy link
Member

Choose a reason for hiding this comment

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

Could we please also add for-in and for-of examples? Just to make this example consistent with other examples from the actual version.

{ code: "function foo(aFoo) { ++aFoo.b; }", options: [{ props: true, ignorePropertyModificationsForRegex: ["^a.*$"] }] },
{ code: "function foo(aFoo) { delete aFoo.b; }", options: [{ props: true, ignorePropertyModificationsForRegex: ["^a.*$"] }] },
{ code: "function foo(a, z) { aFoo.b = 0; x.y = 0; }", options: [{ props: true, ignorePropertyModificationsForRegex: ["^a.*$", "^x.*$"] }] },
{ code: "function foo(aFoo) { aFoo.b.c = 0;}", options: [{ props: true, ignorePropertyModificationsForRegex: ["^a.*$"] }] },
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to have an additional valid test to show that the two options do not intentionally (or unintentionally) disable each other.

For example, a test where props from two different params are modified, one modification is allowed because of ignorePropertyModificationsFor, the other because of ignorePropertyModificationsForRegex.

@platinumazure
Copy link
Member

Mostly unrelated to this PR, but I wonder if we should have an RFC on handling user-supplied regex in ESLint core rules in general.

The RFC process would be useful for helping to outline which rules would benefit and which rules need breaking or large changes, as well as ensuring both the community and the ESLint team have a greater understanding of what changes are needed.

@kaicataldo
Copy link
Member

To piggyback on what @platinumazure says above, it would be great if we could find a way to make this kind of configuration consistent across rules (related discussion here).

@mdjermanovic
Copy link
Member

Just for my understanding, these are two different things?

One is an idea is to make a consistent but probably breaking change in the major version, the other is to have consistent non-breaking changes until the major version?

@platinumazure
Copy link
Member

@mdjermanovic Agreed, but I also want to avoid having too many changes too quickly in this area. Part of me thinks we should just take the time to design this right and be consistent across all rules. If we tell users to use regex one way in a semver-minor release, then tell them to use it in a different way in the next semver-major release, that doesn't look like a good design (in my opinion).

@mdjermanovic
Copy link
Member

Thanks for the clarification! I didn't understand the status of this PR then, thought that the additional option in its current form is accepted as the solution for semver-minor releases, and is waiting to be implemented for this particular rule.

Then, it looks like this PR (and #12305) should wait for RFC?

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the extraordinary patience on this one.

I think the changes identified by @mdjermanovic are mostly nice-to-have and not critical, so I'm okay with merging this in as-is. I'm already planning to open a follow-up PR on this to address a minor performance concern, so I can put in those changes at that time as well.

@platinumazure platinumazure dismissed mdjermanovic鈥檚 stale review October 24, 2019 20:30

This PR has been open for a very long time and I'd like to merge this in. I will address the changes brought up in this review in a follow-up PR

@ilyavolodin ilyavolodin merged commit aac3be4 into eslint:master Oct 24, 2019
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Apr 23, 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 Apr 23, 2020
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