-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Remove "blacklist" terminology #8974
Remove "blacklist" terminology #8974
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon. Please see the contribution instructions for more information. |
If other people don't want to read up on the other PR: From the Clippy team at that time, no one expressed their opposition to that change. But because from then to now, only Manish and I (and Matthias as a member-for-triage-rights 😄) are left, we can get more opinions. I'm still fine with moving forward with this, as long as no compelling arguments should come up against it. @bossmc Please add the old name to this test file to make sure that the deprecation message is printed for the old name:
|
Done, good spot, thanks! One observation I have - this change means that if someone uses the This MR's behaviour matches the equivalent behaviour from the |
Thanks for pointing that out! I think erroring on a deprecated conf value is wrong, because it breaks the configuration of people. I prepared a patch (that can be applied on b3c94c0, that only warns on deprecated conf names and additionally sets the value given to the deprecated name to the new conf name. I attached the patch file: conv.txt For your commits: Can you please first rename files in a separate commit with |
I've had a look at your patch, and it looks reasonable, I feel though that it probably belongs in its own MR and we can make the one-line change in whichever one lands second rather than confusing this (already quite wide-ranging) MR. On the commits - as I understand it git(hub) doesn't look at the individual commits when calculating diffs (especially the style of diff that GitHub uses for MRs) the detection of renames/moves is heuristic. This is even true for a single commit, Additionally, the way these commits are arranged means that each commit builds and passes tests, doing the renames in separate commits will introduce failing commits to the tree, potentially causing issues is Given the above, I propose to leave the commits as is and apologise on behalf of GitHub for the slightly more annoying review job. |
Right, when you look at the diff of a range of commits (like github does), you need the
We don't really care about that. We don't require every commit to build-pass or even test-pass, because we can be sure that the bors commits are always test-pass. So there are already many commits in the tree that aren't build-pass or test-pass.
Makes sense. But we should do that, before merging this. |
☔ The latest upstream changes (presumably #8999) made this pull request unmergeable. Please resolve the merge conflicts. |
Read and use deprecated configuration (as well as emitting a warning) Original change written by `@flip1995` I've simply rebased to master and fixed up the formatting/tests. This change teaches the configuration parser which config key replaced a deprecated key and attempts to populate the latter from the former. If both keys are provided this fails with a duplicate key error (rather than attempting to guess which the user intended). Currently this on affects `cyclomatic-complexity-threshold` -> `cognitive-complexity-threshold` but will also be used in #8974 to handle `blacklisted-names` -> `disallowed-names`. ``` changelog: deprecated configuration keys are still applied as if they were provided as their non-deprecated name. ``` - [x] `cargo test` passes locally - [x] Run `cargo dev fmt`
I've rebased this to |
@bors r+ thanks! |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Picking up where #5896 left off, this renames the
blacklisted_name
lint todisallowed_names
(pluralised for compliance with naming conventions). The old name is still available though is deprecated (both in the lint name, and in the TOML configuration file).This has been proposed/requested a few times, most recently in #7582 where @xFrednet suggested pinging the Clippy team when a PR was created hence...
cc: @rust-lang/clippy
changelog: [
disallowed_names
] lint replacesblacklisted_name