Skip to content

Conversation

@sholderbach
Copy link
Contributor

Fix parsing logic after first whitespace

The way the parse was implemented accepted additional numeric characters
or . after the first valid f64 literal but ignored them.

This permitted more strings than are invalid following a strict grammar
for byte sizes and silently ignores the further symbols without error.

1.0 ...KB
1.0 42.0 B

This change makes those illegal.

1 000 B was also subject to the bad skip_while ignoring the
following 000. In this version of the fix whitespace is not accepted
as a digit separator. So it will raise an error if the user doesn't
explicitly strip the whitespace instead of reporting a wrong value.

The way the parse was implemented accepted additional numeric characters
or `.` after the first valid `f64` literal but ignored them.

This permitted more strings that are invalid following a strict grammar
for byte sizes and silently ignores the further symbols without error.

```
1.0 ...KB
1.0 42.0 B
```
This change makes those illegal.

`1 000 B` was also subject to the bad `skip_while` ignoring the
following `000`. In this version of the fix whitespace is not accepted
as a digit separator. So it will raise an error if the user doesn't
explicitly strip the whitespace instead of reporting a wrong value.
More following the old choices of using `f64::from_str`
Copy link
Member

@robjtede robjtede left a comment

Choose a reason for hiding this comment

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

these test cases look sensible to fix, thanks 👍🏻

@robjtede robjtede merged commit 0e10422 into bytesize-rs:master Feb 9, 2025
11 checks passed
@robjtede robjtede mentioned this pull request Feb 9, 2025
robjtede added a commit that referenced this pull request Feb 9, 2025
robjtede added a commit that referenced this pull request Feb 9, 2025
robjtede added a commit that referenced this pull request Feb 9, 2025
robjtede added a commit that referenced this pull request Feb 9, 2025
robjtede added a commit to Andrew15-5/bytesize that referenced this pull request Feb 10, 2025
* Fix parsing logic after first whitespace

The way the parse was implemented accepted additional numeric characters
or `.` after the first valid `f64` literal but ignored them.

This permitted more strings that are invalid following a strict grammar
for byte sizes and silently ignores the further symbols without error.

```
1.0 ...KB
1.0 42.0 B
```
This change makes those illegal.

`1 000 B` was also subject to the bad `skip_while` ignoring the
following `000`. In this version of the fix whitespace is not accepted
as a digit separator. So it will raise an error if the user doesn't
explicitly strip the whitespace instead of reporting a wrong value.

* Add tests for invalid chars after whitespace

* Add test documenting that whitespace is not a valid digit separator

More following the old choices of using `f64::from_str`

---------

Co-authored-by: Rob Ede <[email protected]>
@sholderbach
Copy link
Contributor Author

Thanks for your maintenance work!

sholderbach added a commit to sholderbach/nushell that referenced this pull request Feb 11, 2025
Closes nushell#14866

Incorporates bytesize-rs/bytesize#59 with
bytesize version 1.3.1

Added test with invalid input that was silently ignored before
sholderbach added a commit to sholderbach/nushell that referenced this pull request Feb 11, 2025
Closes nushell#14866

Incorporates bytesize-rs/bytesize#59 with
bytesize version 1.3.1

Added test with invalid input that was silently ignored before
sholderbach added a commit to nushell/nushell that referenced this pull request Feb 11, 2025
# Description
Closes #14866

Incorporates bytesize-rs/bytesize#59 with
bytesize version 1.3.1

# User-Facing Changes
Now rejected strings
```
"1.3 1.3 kB" | into filesize
"1 420 kB" | into filesize
```
# Tests + Formatting
Added test with invalid input that was silently ignored before
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.

2 participants