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

prevent trailing dots in integers #1267

Merged
merged 1 commit into from
Apr 8, 2024
Merged

prevent trailing dots in integers #1267

merged 1 commit into from
Apr 8, 2024

Conversation

samuelcolvin
Copy link
Member

@samuelcolvin samuelcolvin commented Apr 6, 2024

Change Summary

I noticed while writing the description for #1266 that the docstring for strip_decimal_zeros (now part of clean_int_str) didn't match the actual behaviour:

/// we don't want to parse as f64 then call `float_as_int` as it can loose precision for large ints, therefore
/// we strip `.0+` manually instead, then parse as i64
fn strip_decimal_zeros(s: &str) -> Option<&str> {
if let Some(i) = s.find('.') {
if s[i + 1..].chars().all(|c| c == '0') {
return Some(&s[..i]);
}
}
None
}

Specifically the comment said (which makes sense) that we were stripping .0+ - e.g. dot and one or more zeros. But we were actually stripping dot and zero or more zeros, e.g. ., e.g. '42.' was a valid integer.

This seems clearly incorrect.

NOTE: technically this is a breaking change albeit very small. I think it's okay, but happy to discuss.

cc @adriangb @sydney-runkle @davidhewitt

('042', 42),
('4_2', 42),
('4_2.0', 42),
('04_2.0', 42),
(' 04_2.0 ', 42),
# because zeros are striped before underscores this is not allowed
(' 0_42.0 ', Err('Input should be a valid integer, unable to parse string as an integer')),
Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed this slightly unfortunate but expected format that will fail validation, and added a test to document it. I don't think it needs to be fixed.

Copy link

codspeed-hq bot commented Apr 6, 2024

CodSpeed Performance Report

Merging #1267 will not alter performance

Comparing int-trailing-dot (d1ad954) with main (f636403)

Summary

✅ 155 untouched benchmarks

@samuelcolvin samuelcolvin merged commit b945bcb into main Apr 8, 2024
28 checks passed
@samuelcolvin samuelcolvin deleted the int-trailing-dot branch April 8, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant