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

"MiBy" (mebibytes) prefix unrecognized #39

Open
danielrh opened this issue Apr 25, 2022 · 8 comments
Open

"MiBy" (mebibytes) prefix unrecognized #39

danielrh opened this issue Apr 25, 2022 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@danielrh
Copy link
Contributor

Hi Folks
I tried running this program
https://github.com/danielrh/wise_units_example/blob/mebi/src/main.rs
and this snippet fails

    let over_one_mebibyte = Measurement::try_new(1.125, "MiBy").unwrap();
    let nine_mebi_bit = over_one_mebibyte.convert_to("bit").unwrap();
    assert_eq!(nine_mebi_bit, Measurement::try_new(9* 1024 * 1024, "bit").unwrap());
    eprintln!("M: {} {}", over_one_mebibyte, nine_mebi_bit);

with output

M: 1.125 MBy 9000000 bit
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: ParsingFailed(BadFragment { fragment: "MiBy", position: 0 })', src/main.rs:10:65

I'd like to be able to use the SI Mebibyte units in my application, but the prefix does not seem to be recognized

I see it in the parser, but I can't find any tests for this sort of unit-- is there something I can do to fix the issue?

@danielrh
Copy link
Contributor Author

I incorporated the test into a lower level of wise_units, but the problem seems to be somewhere in pest's generated parser:
The patch here

diff --git a/api/src/parser/terms/mapper.rs b/api/src/parser/terms/mapper.rs
index 3ab4ad4..dea2d05 100644
--- a/api/src/parser/terms/mapper.rs
+++ b/api/src/parser/terms/mapper.rs
@@ -102,6 +102,12 @@ mod tests {
     );
     validate_interpret!(validate_interpret_kilometer, "km", term!(Kilo, Meter));
 
+    validate_interpret!(validate_interpret_kilobyte, "kBy", term!(Kilo, Byte));
+    validate_interpret!(validate_sec_interpret_klobyte, "KBY", term!(Kilo, Byte));
+    validate_interpret!(validate_sec_interpret_kibibyte, "KIBBY", term!(Kibi, Byte));
+    validate_interpret!(validate_interpret_kibibyte, "KiBy", term!(Kibi, Byte));
+
+
     // Slash terms
     validate_interpret!(
         validate_interpret_meter_per_second,

kibibyte_test.txt

results in this internal error.
Stepping through it seems to be deep inside pest's parser and state tracking.

running 6 tests
test parser::terms::mapper::tests::validate_interpret_kibibyte ... FAILED
test parser::terms::mapper::tests::validate_interpret_kilometer_per_second ... ok
test parser::terms::mapper::tests::validate_interpret_kilometer ... ok
test parser::terms::mapper::tests::validate_sec_interpret_kibibyte ... FAILED
test parser::terms::mapper::tests::validate_sec_interpret_klobyte ... ok
test parser::terms::mapper::tests::validate_interpret_kilobyte ... ok

failures:

---- parser::terms::mapper::tests::validate_interpret_kibibyte stdout ----
prefix wat
thread 'parser::terms::mapper::tests::validate_interpret_kibibyte' panicked at 'called `Result::unwrap()` on an `Err` value: UnknownUnitString("Ki")', src/parser/terms/mapper.rs:108:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- parser::terms::mapper::tests::validate_sec_interpret_kibibyte stdout ----
thread 'parser::terms::mapper::tests::validate_sec_interpret_kibibyte' panicked at 'called `Result::unwrap()` on an `Err` value: BadFragment { fragment: "KIBBY", position: 0 }', src/parser/terms/mapper.rs:107:5


failures:
    parser::terms::mapper::tests::validate_interpret_kibibyte
    parser::terms::mapper::tests::validate_sec_interpret_kibibyte

@turboladen
Copy link
Contributor

@danielrh Thanks for the ticket! And yeah, it looks like there's a precedence bug in the pest-generated parser. Thanks for investigating--I pulled your test into a branch, and it's passing now. I'll want to add some more tests to this branch, but if you have the time, would you mind trying out my branch to see if it fixes your issues? feature/gh-39-parser-prefix-ordering

@turboladen turboladen self-assigned this Apr 26, 2022
@turboladen turboladen added the bug Something isn't working label Apr 26, 2022
@danielrh
Copy link
Contributor Author

Thanks for moving quickly here!
MiBy works wonderfully but KiBy fails

prefix wat
thread 'units::test::test_conversion_polynomials' panicked at 'called Result::unwrap() on an Err value: Custom { kind: InvalidData, error: "ParsingFailed(UnknownUnitString("Ki"))" }', experimental_server/units.rs:86:72
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

@danielrh
Copy link
Contributor Author

Gi fails as well but Ti works, interestingly

@danielrh
Copy link
Contributor Author

Do you know why Kibi isn’t appearing in this autogenerated table?

https://github.com/agrian-inc/wise_units/blob/d8893801edea6cf2debad875b75e304005d158c5/api/src/parser/prefix.rs#L222

@danielrh
Copy link
Contributor Author

Hmmm the top of symbol.pest with the current change says

//-----------------------------------------------------------------------------
// DO NOT EDIT THIS FILE!
// This is generated at compile time.
//-----------------------------------------------------------------------------

Is that related to the missing table entry in parser.rs?

@danielrh
Copy link
Contributor Author

I may have a fix in #40
Would welcome feedback or thoughts there....
still unusre about the DO NOT EDIT THIS FILE message I flagged above

turboladen added a commit that referenced this issue Apr 27, 2022
…ring

Add more tests and fix bug in prefix.rs
@turboladen
Copy link
Contributor

turboladen commented Apr 27, 2022

Hmmm the top of symbol.pest with the current change says

//----------------------------------------------------------------------------- // DO NOT EDIT THIS FILE! // This is generated at compile time. //-----------------------------------------------------------------------------

Is that related to the missing table entry in parser.rs?

Unfortunately, that used to be the case, but is currently a misnomer. I diverged from using the repo's atom_generator some time ago. Embarrassingly, I built hacked together the generation via handlebars--I'd like to say this was prior to the quote crate, but I'm not sure. ...in any case, it became more work with little gain to keep that working properly, since that code doesn't need to be (re)generated often (really, only when the UCUM spec gets updated). All that to say, there is some cruft left in the codebase that should be cleaned up related to this; you're fine to manually edit.

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

No branches or pull requests

2 participants