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

[refurb] Handle non-finite decimals in verbose-decimal-constructor (FURB157) #14596

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dylwil3
Copy link
Collaborator

@dylwil3 dylwil3 commented Nov 26, 2024

This PR extends the Decimal parsing used in verbose-decimal-constructor (FURB157) to better handle non-finite Decimal objects, avoiding some false negatives.

Closes #14587

Copy link
Contributor

github-actions bot commented Nov 26, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser MichaReiser added the bug Something isn't working label Nov 26, 2024
| "-infinity"
| "nan"
| "+nan"
| "-nan"
) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to your fix: To avoid an allocation in many cases, it might be worth trying an exact match first before calling to_lowercase.

let trimmed = float.value.to_str().trim();

if is_inf_infinity_or_nan(trimmed) {
	return;
}

let normalized_float_string = trimmed.to_lowercase();

if is_inf_infinity_or_nan(&normalized_flaot_string) {
	return;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I'm misunderstanding but I think we have to allocate anyway: we only get to escape early here if float.value is not inf, infinity, or nan (after trimming and lowercasing). So if we first checked that the trimmed version wasn't "nan" etc., we would still have to rule out the case where it's "NaN".

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's just that we do it less often. I guess the overall idea is to spell out all common variations so that the rule can short-circuit for almost all valid-cases and only allocates for uncommon spellings (e.g. nAn).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FURB157 has false negatives for non-finite float strings
2 participants