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: deprecate id-blacklist rule #13465

Merged
merged 5 commits into from Jul 14, 2020

Conversation

@dimitropoulos
Copy link
Contributor

@dimitropoulos dimitropoulos commented Jul 7, 2020

Prerequisites checklist

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

[x] Documentation update
[x] 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 changes did you make? (Give an overview)

I noticed that in the haste to rename id-blacklist to id-denylist, id-blacklist was not deprecated. This PR aims to mark id-blacklist as deprecated and document this change.

Regarding documentation, note that some tooling of tooling (well, tooling I use, anyway) expects the rule to have documentation matching the name of the rule.

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

Just to make sure this was done in the desired/appropriate manner.

@eslint eslint bot added the triage label Jul 7, 2020
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jul 7, 2020

The decision not to deprecate the rule was a conscious one. We thought that creating a URL redirect on the website would be enough. I'm not against marking the rule as deprecated if the direction we took is breaking things, but do you mind sharing a concrete example of what's breaking?

@kaicataldo kaicataldo added evaluating rule and removed triage labels Jul 7, 2020
@dimitropoulos
Copy link
Contributor Author

@dimitropoulos dimitropoulos commented Jul 7, 2020

sure: for example I use https://github.com/sarbbottam/eslint-find-rules for some configs I maintain (for example, one is eslint-config-intense) and it will ask me to define rules for both id-blacklist and id-denylist since they are not deprecated.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jul 7, 2020

Thanks for sharing that. @eslint/eslint-tsc Thoughts on how we should proceed here?

@nzakas
Copy link
Member

@nzakas nzakas commented Jul 7, 2020

I think this makes sense. We were operating on the theory that there wouldn't be problems with making this switch silently, but that now appears not to be the case. Deprecation should solve the problem.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jul 10, 2020

Sounds good to me. @btmills @mysticatea @mdjermanovic thoughts?

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Jul 10, 2020

Deprecation sounds good because it looks confusable for tools if the same rule exists as two different names and both are not deprecated.

I have a small concern about the deprecated rule imports the living rule. We have made the code of deprecated rules frozen until this. Therefore, I feel odd a bit if we will continue to update the new deprecated rule. But it's not a strong feeling because this is rename.

@dimitropoulos
Copy link
Contributor Author

@dimitropoulos dimitropoulos commented Jul 10, 2020

@mysticatea that's fair. I was trying to import id-denylist's code into id-blacklist to reduce duplication, but I'd be happy to copy-pasta if that serves the use-case better for freezing a deprecated rule.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jul 10, 2020

I also think it would be consistent to copy & paste the code, and thus freeze the deprecated rule.

@btmills
Copy link
Member

@btmills btmills commented Jul 10, 2020

Looks like y'all have this covered. 👍 to deprecation since it seems like the best path forward, and while I respect the DRYness of the current implementation, 👍 to copy & paste since we have previous deprecations as precedent.

@dimitropoulos
Copy link
Contributor Author

@dimitropoulos dimitropoulos commented Jul 11, 2020

Thanks all: I'll definitely jump on that soon. While I do so, may I suggest that we document this rule about deprecated rules being frozen in developer docs somewhere. I think it's a great policy to have and it seems like it's worth making official.

I'd be happy to make a issue+PR if so (we can discuss where it should live in the issue maybe?)

docs/rules/id-blacklist.md Outdated Show resolved Hide resolved
@dimitropoulos dimitropoulos force-pushed the dimitropoulos:id-blacklist-deprecated branch from 8d0b742 to b932195 Jul 11, 2020
@dimitropoulos
Copy link
Contributor Author

@dimitropoulos dimitropoulos commented Jul 11, 2020

ok! looks like this is all set (as far as I'm aware)!

lib/rules/id-blacklist.js Show resolved Hide resolved
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jul 11, 2020

Thanks all: I'll definitely jump on that soon. While I do so, may I suggest that we document this rule about deprecated rules being frozen in developer docs somewhere. I think it's a great policy to have and it seems like it's worth making official.

I think the policy is already documented here:

https://eslint.org/docs/user-guide/rule-deprecation

Maybe it isn't explicit enough that the behavior of the rule will never change?

Hm, actually if it happens that a deprecated rule uses some shared helpers like astUtils, its behavior can be changed indirectly.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jul 11, 2020

It seems there is a consensus to do this, so I'm marking this PR as accepted.

@mdjermanovic mdjermanovic added accepted and removed evaluating labels Jul 11, 2020
@mdjermanovic mdjermanovic changed the title Fix: deprecates id-blacklist (and documents deprecation) Update: deprecate id-blacklist rule Jul 11, 2020
Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

@kaicataldo kaicataldo merged commit f4d7b9e into eslint:master Jul 14, 2020
12 checks passed
12 checks passed
Verify Files
Details
Test (ubuntu-latest, 14.x)
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 10.12.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jul 14, 2020

Thanks for contributing!

@dimitropoulos dimitropoulos deleted the dimitropoulos:id-blacklist-deprecated branch Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.