naomijub / edn-rs

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

Numbers #110

Closed Grinkers closed 10 months ago

Grinkers commented 11 months ago

WIP for #106

codecov-commenter commented 11 months ago

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (e78eac9) 71.39% compared to head (23cd63f) 71.38%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #110 +/- ## ========================================== - Coverage 71.39% 71.38% -0.02% ========================================== Files 8 8 Lines 944 940 -4 ========================================== - Hits 674 671 -3 + Misses 270 269 -1 ``` | [Files](https://app.codecov.io/gh/naomijub/edn-rs/pull/110?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/110?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Julia+Naomi#diff-c3JjL2Rlc2VyaWFsaXplL3BhcnNlLnJz) | `93.28% <100.00%> (ø)` | | | [src/edn/utils/mod.rs](https://app.codecov.io/gh/naomijub/edn-rs/pull/110?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Julia+Naomi#diff-c3JjL2Vkbi91dGlscy9tb2QucnM=) | `66.66% <ø> (+7.84%)` | :arrow_up: | | [src/json/mod.rs](https://app.codecov.io/gh/naomijub/edn-rs/pull/110?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Julia+Naomi#diff-c3JjL2pzb24vbW9kLnJz) | `100.00% <100.00%> (ø)` | | | [src/lib.rs](https://app.codecov.io/gh/naomijub/edn-rs/pull/110?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Julia+Naomi#diff-c3JjL2xpYi5ycw==) | `100.00% <ø> (ø)` | | | [src/serialize/mod.rs](https://app.codecov.io/gh/naomijub/edn-rs/pull/110?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Julia+Naomi#diff-c3JjL3NlcmlhbGl6ZS9tb2QucnM=) | `75.20% <ø> (ø)` | | | [src/deserialize/mod.rs](https://app.codecov.io/gh/naomijub/edn-rs/pull/110?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Julia+Naomi#diff-c3JjL2Rlc2VyaWFsaXplL21vZC5ycw==) | `59.67% <66.66%> (+0.47%)` | :arrow_up: | | [src/edn/mod.rs](https://app.codecov.io/gh/naomijub/edn-rs/pull/110?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Julia+Naomi#diff-c3JjL2Vkbi9tb2QucnM=) | `60.27% <59.25%> (-1.13%)` | :arrow_down: | | [src/edn/utils/index.rs](https://app.codecov.io/gh/naomijub/edn-rs/pull/110?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Julia+Naomi#diff-c3JjL2Vkbi91dGlscy9pbmRleC5ycw==) | `19.78% <23.52%> (-0.22%)` | :arrow_down: |

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

naomijub commented 11 months ago

Do you want us to review as you push or wait til the end?

Grinkers commented 11 months ago

Either way.

I'll poke at f64 later this week. I think I'll push BigInt stuff off for later (I don't need it). I'll see how f64 with ordered-float goes with an optional default-features = false to disable sets. With feature flags, I think it makes sense to either do BigInt correctly or not at all.

naomijub commented 11 months ago

Sounds good

Grinkers commented 10 months ago

ordered-float works with default-features = false. The only additional dependency is num-traits.

cargo license --avoid-build-deps --avoid-dev-deps --filter-platform x86_64-unknown-linux-gnu
Apache-2.0 OR MIT (1): num-traits
MIT (2): edn-rs, ordered-float

I've updated the OP todo list to have sets be optional with no additional dependency and removed BigInt for a future feature.

Grinkers commented 10 months ago

Pushed an implementation of --no-default-features. It ended up being way more messy than I was expecting. Two things I really don't like are

  1. Test organization. I wanted to take a pass at error handling as a separate issue, probably along with a borrow feature. I'd probably revisit it then
  2. Currently cargo t --no-default-features fails because it is compiling the docs in src/macros/mod.rs. This could be "fixed" with doctest = false, but not really ideal at all. Thoughts?

https://doc.rust-lang.org/cargo/commands/cargo-test.html for reference

I'll have to study how other crates handle lots of features.

naomijub commented 10 months ago

The macro_rules section is something that I particularly feel as less useful now a day. If that Is the issue, I would definitely consider removing that macro

Grinkers commented 10 months ago

The macro_rules section is something that I particularly feel as less useful now a day. If that Is the issue, I would definitely consider removing that macro

I don't mind supporting the macro. It's just the rust in the docs end up getting run as part of tests, but it uses sets.

Next week I'll be using this branch for my project and I'll see how it goes. I need the better number handling, but don't need sets (and thus no reason to bring in a dependency).

Grinkers commented 10 months ago

This got too big. Closing it and breaking things apart. I'll keep the branch alive until everything else gets settled/merged nicely.