naomijub / edn-rs

[DEPRECATED]: Crate to parse and emit EDN
https://crates.io/crates/edn-rs
MIT License
81 stars 14 forks source link

Implementation of non-dec number parsing, trying to match clojure.edn/read-string as much as possible. #103

Closed Grinkers closed 1 year ago

Grinkers commented 1 year ago

Resolves #100, both the mentioned non-dec parsing but also throwing errors when encountering invalid numbers instead of giving a bad result.

Using rust's from_str_radix, supporting all radix from 2 to 36, just like clojure.edn/read-string ended up not being too painful.

I don't have a super strong opinion other than needing 0x support before picking up this crate for my projects. That being said, I can only imagine future problems/compatibility by adding rust's additional 0b and 0o patterns.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.83% :tada:

Comparison is base (8353fc4) 69.60% compared to head (e671af5) 70.44%.

:exclamation: Current head e671af5 differs from pull request most recent head f042cbe. Consider uploading reports for the commit f042cbe to get more accurate results

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #103 +/- ## ========================================== + Coverage 69.60% 70.44% +0.83% ========================================== Files 8 8 Lines 931 954 +23 ========================================== + Hits 648 672 +24 + Misses 283 282 -1 ``` | [Files Changed](https://app.codecov.io/gh/naomijub/edn-rs/pull/103?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Julia+Naomi) | Coverage Δ | | |---|---|---| | [src/deserialize/parse.rs](https://app.codecov.io/gh/naomijub/edn-rs/pull/103?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Julia+Naomi#diff-c3JjL2Rlc2VyaWFsaXplL3BhcnNlLnJz) | `92.55% <100.00%> (+1.04%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Grinkers commented 1 year ago

Instead of commenting on a lot of the repeated areas, I was trying to match clojure.edn/read-string as much as possible. Here's some input/outputs

(clojure.edn/read-string "2r10100101")
165

(clojure.edn/read-string "16r2a")
42

(clojure.edn/read-string "16r2a")
42

(clojure.edn/read-string "-16r2a")
-42

(clojure.edn/read-string "-0X2A")
-42

(clojure.edn/read-string "-36rabcxyz")
-623741435

(clojure.edn/read-string "42invalid123")
java.lang.NumberFormatException: Invalid number: 42invalid123 [at <repl>:5:1]

(clojure.edn/read-string "0xxyz123")
java.lang.NumberFormatException: Invalid number: 0xxyz123 [at <repl>:1:1]

(clojure.edn/read-string "42rabcxzy")
java.lang.NumberFormatException: Radix out of range [at <repl>:2:1]

(clojure.edn/read-string "42crazyrabcxzy")
java.lang.NumberFormatException: Invalid number: 42crazyrabcxzy [at <repl>:3:1]

https://github.com/edn-format/edn#symbols

Symbols begin with a non-numeric character

I realize a lot of this radix stuff is pretty messy (both readability and for parsing) and is technically off-spec, so I would understand if you don't want to take on the burden. I've become dependent (perhaps wrongly so!) on the more liberal behavior of clojure.edn/read-string, so I tried to match it as close as possible. This certainly seems like a pain point for EDN.

naomijub commented 1 year ago

Sorry for the delay, crazy times. I agree with you point, will merge