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

Read and use deprecated configuration (as well as emitting a warning) #9252

Merged
merged 2 commits into from
Jul 29, 2022
Merged

Read and use deprecated configuration (as well as emitting a warning) #9252

merged 2 commits into from
Jul 29, 2022

Conversation

bossmc
Copy link
Contributor

@bossmc bossmc commented Jul 27, 2022

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.
  • cargo test passes locally
  • Run cargo dev fmt

Co-authored-by: Andy Caldwell <andycaldwell@microsoft.com>
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 27, 2022
@flip1995
Copy link
Member

I only vaguely remember writing this 😅 Thanks for seeing it to an end!

@Jarcho
Copy link
Contributor

Jarcho commented Jul 27, 2022

One small issue. This won't warn about duplicate fields when the old name appears after the new name. It's not a big deal as you still get the warning about the deprecated field.

Otherwise LGTM.

@bossmc
Copy link
Contributor Author

bossmc commented Jul 28, 2022

I've added a check to handle duplicates in either order and added a test for duplicates. I also extended the error message to be clear what's happening if one of the duplicates is using the deprecated name (though this only works on one direction).

@Jarcho
Copy link
Contributor

Jarcho commented Jul 29, 2022

Thank you. That should be good enough. The deprecation warning mentions both names in either case.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 29, 2022

📌 Commit ea25ef1 has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 29, 2022

⌛ Testing commit ea25ef1 with merge 53a09d4...

@bors
Copy link
Contributor

bors commented Jul 29, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 53a09d4 to master...

@bors bors merged commit 53a09d4 into rust-lang:master Jul 29, 2022
@bossmc bossmc deleted the use-deprecated-config branch September 13, 2023 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants