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

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

Merged
merged 3 commits into from
Nov 20, 2023

Conversation

Grinkers
Copy link
Collaborator

@Grinkers Grinkers commented Oct 29, 2023

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
Copy link

codecov-commenter commented Oct 29, 2023

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%.

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

Additional details and impacted files
@@            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 Coverage Δ
src/deserialize/mod.rs 59.67% <100.00%> (+0.47%) ⬆️
src/deserialize/parse.rs 93.28% <ø> (ø)
src/edn/utils/index.rs 19.78% <ø> (ø)
src/json/mod.rs 100.00% <100.00%> (ø)
src/lib.rs 100.00% <ø> (ø)
src/edn/mod.rs 60.27% <75.00%> (-0.87%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Grinkers Grinkers changed the title Double is now f64, leveraging rust's IEEE-754 Double is now f64, leveraging rust's IEEE-754 + optional feature "sets" Oct 29, 2023
@Grinkers
Copy link
Collaborator Author

Grinkers commented Nov 7, 2023

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

@Grinkers Grinkers marked this pull request as ready for review November 7, 2023 10:21
@Grinkers Grinkers merged commit e874e10 into naomijub:master Nov 20, 2023
5 checks passed
@Grinkers Grinkers deleted the floats branch November 20, 2023 14:12
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.

3 participants