-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[ruff] Implement check for Decimal called with a float literal (RUF032) #12909
[ruff] Implement check for Decimal called with a float literal (RUF032) #12909
Conversation
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
RUF032 | 25 | 25 | 0 | 0 | 0 |
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.
Nice. I'd love to get some input from @AlexWaygood but this looks great
crates/ruff_linter/src/rules/ruff/rules/float_literal_decimal.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/float_literal_decimal.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/float_literal_decimal.rs
Outdated
Show resolved
Hide resolved
|
||
fn fix_float_literal(range: TextRange, float_literal: &str) -> Fix { | ||
let content = format!("\"{float_literal}\""); | ||
Fix::unsafe_edit(Edit::range_replacement(content, range)) |
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.
What's your reasoning for making it an unsafe edit. Is it because it changes the precision of the values and, therefore, could result in runtime changes?
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.
I think an unsafe edit is appropriate here... in most cases, you'll be changing the underlying value of the Decimal
object being constructed. There might be code elsewhere that implicitly depended on the previous, "wrong" value that was caused by constructing the Decimal
instance from the float, so it's entirely possible that fixing this error could unexpectedly cause your code to break.
@kbaskett248, it would be great if you could add one or two sentences to the docs explaining why this is an unsafe fix, like how we do with other rules.
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.
Honestly it was mostly unfamiliarity with the apis, but @AlexWaygood brings up a good point.
crates/ruff_linter/src/rules/ruff/rules/decimal_from_float_literal.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Alex Waygood <[email protected]>
crates/ruff_linter/src/rules/ruff/rules/decimal_from_float_literal.rs
Outdated
Show resolved
Hide resolved
CodSpeed Performance ReportMerging #12909 will not alter performanceComparing Summary
|
You can ignore that, it's been super flaky recently |
Thanks for your help with this @AlexWaygood and @MichaReiser! Any other updates you'd like me to make? |
fn fix_float_literal(range: TextRange, float_literal: &str) -> Fix { | ||
let content = format!("\"{float_literal}\""); | ||
Fix::unsafe_edit(Edit::range_replacement(content, range)) |
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.
One last thing. Here we should use the quote style that the user is generally using for the rest of their file, which won't necessarily be double quotes if they're not using autoformatting. We can do this with the checker.stylist()
:
fn fix_float_literal(range: TextRange, float_literal: &str) -> Fix { | |
let content = format!("\"{float_literal}\""); | |
Fix::unsafe_edit(Edit::range_replacement(content, range)) | |
fn fix_float_literal(range: TextRange, float_literal: &str, stylist: &Stylist) -> Fix { | |
let quote = stylist.quote(); | |
let content = format!("{quote}{float_literal}{quote}"); | |
Fix::unsafe_edit(Edit::range_replacement(content, range)) |
You'll just want to pass checker.stylist()
into this function at the callsite, and you'll want to add use ruff_python_codegen::Stylist;
to the top of the file so that you can use it here in the type signature
a4964d9
to
9a8fd54
Compare
Thank you both for your help getting this in! |
Thanks for the great contribution! |
Summary
This PR adds a new check for a
Decimal
call with a float argument. As described in #11840, this is a problem when precision is important, which is usually whyDecimal
is used in the first place. To pull an example by @mayanyax from the issue:The check supports positive and negative floats. It also includes a fix that wraps the float in quotes.
Test Plan
Test cases were added for the new rule.