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

Conf macro improvements #7150

Merged
merged 2 commits into from
May 2, 2021
Merged

Conf macro improvements #7150

merged 2 commits into from
May 2, 2021

Conversation

camsteffen
Copy link
Contributor

changelog: Allow default_trait_access in macros

Mainly this is a change to use serde as in Manually implementing Deserialize for a struct, which opens the door for a cleaner implementation overall.

  • Allow default_trait_access in macros (tangential, but used in this PR)
  • Deserialize into TryConf { conf, errors } instead of using a global ERRORS variable.
  • Improve the define_Conf! macro
    • Remove the redundant string literal (name, "name", ..)
    • Support deprecated configs with #[conf_deprecated(message)]. Message shows in error.
    • Make the default value optional. Use Default::default() if omitted.
  • Invalid config value error now shows the key (see test output)
  • Cleaner impl Default for Conf (no toml::from_str(""))

@rust-highfive
Copy link

r? @llogiq

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 1, 2021
@llogiq
Copy link
Contributor

llogiq commented May 2, 2021

Thank you, that is a solid improvement!

@bors r+

@bors
Copy link
Contributor

bors commented May 2, 2021

📌 Commit 1e22e56 has been approved by llogiq

@bors
Copy link
Contributor

bors commented May 2, 2021

⌛ Testing commit 1e22e56 with merge 019dfb9...

@bors
Copy link
Contributor

bors commented May 2, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 019dfb9 to master...

@bors bors merged commit 019dfb9 into rust-lang:master May 2, 2021
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This broke the documentation generation script. The script works with regexes. I assume the change from , to = is the problem here. See commit 0ae8be3

Also cc @xFrednet making you aware that the conf macro changed a bit, since I think you already have started / finished work on this for the metadata collection monster.

clippy_lints/src/utils/conf.rs Show resolved Hide resolved
@xFrednet
Copy link
Member

xFrednet commented May 2, 2021

Yup, my implementation will be slightly effected by this change, but still very manageable. The ping was definitely good! <3

@camsteffen Could you also adapt the documentation with the new configuration syntax. I think the only effected area should be this: Adding configuration to a lint

@camsteffen
Copy link
Contributor Author

Whoops. Yes I can fix those two things.

@flip1995
Copy link
Member

flip1995 commented May 2, 2021

@camsteffen When working on the documentation, I recommend using cargo dev serve which live-updates the documentation page in your browser when change something.

bors added a commit that referenced this pull request May 3, 2021
`Conf` macro improvements part 2

changelog: none

Follow-up to #7150

I made the default value required again for `define_Conf!` so that it can be parsed by the magic Python. I guess it's just as well for readability.

r? `@flip1995`
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.

6 participants