Skip to content

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

Merged
bors merged 2 commits into
rust-lang:masterfrom
Metaswitch:use-deprecated-config
Jul 29, 2022
Merged

Read and use deprecated configuration (as well as emitting a warning)#9252
bors merged 2 commits into
rust-lang:masterfrom
Metaswitch:use-deprecated-config

Conversation

@bossmc
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Member

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

@Jarcho
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

bors commented Jul 29, 2022

⌛ Testing commit ea25ef1 with merge 53a09d4...

@bors
Copy link
Copy Markdown
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