naomijub / edn-rs

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

Double is now f64, leveraging rust's IEEE-754 + optional feature "sets" #114

Closed Grinkers closed 11 months ago

Grinkers commented 1 year ago

This is a continuation of #110.

This should be after #113. Not worth dealing with merge conflicts, I'll just rebase to keep the history clean.

codecov-commenter commented 1 year ago

Codecov Report

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

Comparison is base (36f9a1d) 71.27% compared to head (2932993) 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 #114 +/- ## ========================================== + Coverage 71.27% 71.38% +0.10% ========================================== Files 8 8 Lines 947 940 -7 ========================================== - Hits 675 671 -4 + Misses 272 269 -3 ``` | [Files](https://app.codecov.io/gh/naomijub/edn-rs/pull/114?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Julia+Naomi) | Coverage Δ | | |---|---|---| | [src/deserialize/mod.rs](https://app.codecov.io/gh/naomijub/edn-rs/pull/114?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% <100.00%> (+0.47%)` | :arrow_up: | | [src/deserialize/parse.rs](https://app.codecov.io/gh/naomijub/edn-rs/pull/114?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% <ø> (ø)` | | | [src/edn/utils/index.rs](https://app.codecov.io/gh/naomijub/edn-rs/pull/114?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% <ø> (ø)` | | | [src/json/mod.rs](https://app.codecov.io/gh/naomijub/edn-rs/pull/114?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/114?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/edn/mod.rs](https://app.codecov.io/gh/naomijub/edn-rs/pull/114?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% <75.00%> (-0.87%)` | :arrow_down: |

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

Grinkers commented 1 year ago

Rebased and pushed, while allowing all tests to pass. I think the implementation is ready for review, but I'm not too happy with how many #[cfg")] are scattered about. Some amount is unavoidable, but in particular the tests tests are a little ugly. I took a look at some other crates and the common pattern there seems to be something like

// some_tests.rs
mod generic_tests;
#[cfg(feature = "sets")]
mod tests_with_sets;
...

I wanted to take a look at Error handling and a borrow impl (less heap allocations, requiring a walker). I think that might be the time/place to fix/clean this up.
https://github.com/rust-embedded-community/serde-json-core/blob/9deb2b91ea737df79f33f7536587b59ed5f141ee/src/de/mod.rs#L23 for reference for what I sort of had in mind. These can now be added now that we have non_exhaustive