-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Derive message formats macro support to string #14093
Derive message formats macro support to string #14093
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Just three places where I think you went slightly too far ;)
crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_simplify/rules/zip_dict_keys_and_values.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pygrep_hooks/rules/blanket_noqa.rs
Outdated
Show resolved
Hide resolved
Fair point, reverted those three cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. It might be worth @MichaReiser -- or somebody else who knows their Rust macros a little better -- also taking a look at the changes in ruff_macros
, though 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't mind the change and it certainly helps that people don't learn the "bad" pattern of using format! over
to_string` if there are no arguments.
At the other hand, to_string
calls Display
. So format!
and to_string
are equivalent. So this is a pure "esthetic" change. I'm okay with it because it doesn't add too much complexity to the macros and my main concern is with the macro itself (but that's a whole other story).
* main: (39 commits) Also remove trailing comma while fixing C409 and C419 (astral-sh#14097) Re-enable clippy `useless-format` (astral-sh#14095) Derive message formats macro support to string (astral-sh#14093) Avoid cloning `Name` when looking up function and class types (astral-sh#14092) Replace `format!` without parameters with `.to_string()` (astral-sh#14090) [red-knot] Do not panic when encountering string annotations (astral-sh#14091) [red-knot] Add MRO resolution for classes (astral-sh#14027) [red-knot] Remove `Type::None` (astral-sh#14024) Cached inference of all definitions in an unpacking (astral-sh#13979) Update dependency uuid to v11 (astral-sh#14084) Update Rust crate notify to v7 (astral-sh#14083) Update cloudflare/wrangler-action action to v3.11.0 (astral-sh#14080) Update dependency mdformat-mkdocs to v3.1.1 (astral-sh#14081) Update pre-commit dependencies (astral-sh#14082) Update dependency ruff to v0.7.2 (astral-sh#14077) Update NPM Development dependencies (astral-sh#14078) Update Rust crate thiserror to v1.0.67 (astral-sh#14076) Update Rust crate syn to v2.0.87 (astral-sh#14075) Update Rust crate serde to v1.0.214 (astral-sh#14074) Update Rust crate pep440_rs to v0.7.2 (astral-sh#14073) ...
Summary
Follow-up of #14090: the
derive_message_formats
macro previously did not supportString::to_string
. This PR changes the macro to requireString::to_string
instead offormat!
when no formatting parameters are provided.There is room for further expansion of this macro, as this pattern used in #14090 is not yet supported for the message method:
Test Plan
cargo test
andcargo expand
, e.g.cargo expand rules::flake8_bandit::rules::jinja2_autoescape_false -p ruff_linter