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

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-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jul 7, 2020
@kaicataldo
Copy link
Member

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 The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Jul 7, 2020
@dimitropoulos
Copy link
Contributor Author

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

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

@nzakas
Copy link
Member

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

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

@mysticatea
Copy link
Member

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

@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

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

@btmills
Copy link
Member

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

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
Copy link
Contributor Author

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

@mdjermanovic
Copy link
Member

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

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

@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 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

Choose a reason for hiding this comment

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

LGTM, thanks!

@kaicataldo kaicataldo merged commit f4d7b9e into eslint:master Jul 14, 2020
@kaicataldo
Copy link
Member

Thanks for contributing!

@dimitropoulos dimitropoulos deleted the id-blacklist-deprecated branch July 14, 2020 12:01
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jan 11, 2021
@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 Jan 11, 2021
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 rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants