Skip to content

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

('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
@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

1 participant