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
Update: deprecate id-blacklist rule #13465
Conversation
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? |
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 |
Thanks for sharing that. @eslint/eslint-tsc Thoughts on how we should proceed here? |
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. |
Sounds good to me. @btmills @mysticatea @mdjermanovic thoughts? |
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. |
@mysticatea that's fair. I was trying to import |
I also think it would be consistent to copy & paste the code, and thus freeze the deprecated rule. |
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. |
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?) |
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
8d0b742
to
b932195
Compare
ok! looks like this is all set (as far as I'm aware)! |
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 |
It seems there is a consensus to do this, so I'm marking this PR as accepted. |
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!
Thanks for contributing! |
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
toid-denylist
,id-blacklist
was not deprecated. This PR aims to markid-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.