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

@bossmc bossmc commented Jul 27, 2022

Copy link
Copy Markdown
Contributor

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

Jarcho commented Jul 27, 2022

Copy link
Copy Markdown
Contributor

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

bossmc commented Jul 28, 2022

Copy link
Copy Markdown
Contributor Author

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

Jarcho commented Jul 29, 2022

Copy link
Copy Markdown
Contributor

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

@bors r+

@bors

bors commented Jul 29, 2022

Copy link
Copy Markdown
Contributor

📌 Commit ea25ef1 has been approved by Jarcho

It is now in the queue for this repository.

@bors

bors commented Jul 29, 2022

Copy link
Copy Markdown
Contributor

⌛ Testing commit ea25ef1 with merge 53a09d4...

@bors

bors commented Jul 29, 2022

Copy link
Copy Markdown
Contributor

☀️ 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