search

naomijub / edn-rs

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

API reduction #121

Closed Grinkers closed 9 months ago

Grinkers commented 9 months ago

Discussion bits are scattered about, but mostly in #120 (which was growing beyond scope).

Grinkers commented 9 months ago

@naomijub I'm able to merge into master without a review. That's probably not what we want. Is it possible to add all the restrictions on master but not other branches?

naomijub commented 9 months ago

Will do, I probably can review it after my lunch and then add ruleset to main

codecov-commenter commented 9 months ago

Codecov Report

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

Comparison is base (d852751) 71.53% compared to head (96109d2) 71.28%.

Files Patch % Lines
src/edn/utils/index.rs 50.00% 2 Missing :warning:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #121 +/- ## ========================================== - Coverage 71.53% 71.28% -0.26% ========================================== Files 8 8 Lines 938 867 -71 ========================================== - Hits 671 618 -53 + Misses 267 249 -18 ```

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

Grinkers commented 9 months ago

I sneaked in test refactoring here because it was causing noise for #120.

I'm guessing for historical reasons it made it easier to work with, but it was testing internal structures that I intend to rewrite, so I converted them to integration tests in https://github.com/naomijub/edn-rs/pull/121/commits/78bdf1cbc201b0295a44c04ff6633e73a6b827bb

If it's ok, I'll continue doing this as I run into things like in https://github.com/naomijub/edn-rs/pull/121/commits/6db3875a4b4afe1ecc2588ff328c301ad2c6c396. A lot of the tests are actually using only public interfaces so can live in tests/. As you probably noticed, there's a lot less #[cfg(feature = "sets")] scattered all over too.

I think going forward cut/paste. I'll keep them in separate commits so it's easy to track.

naomijub commented 9 months ago

I sneaked in test refactoring here because it was causing noise for #120.

I'm guessing for historical reasons it made it easier to work with, but it was testing internal structures that I intend to rewrite, so I converted them to integration tests in 78bdf1c

If it's ok, I'll continue doing this as I run into things like in 6db3875. A lot of the tests are actually using only public interfaces so can live in tests/. As you probably noticed, there's a lot less #[cfg(feature = "sets")] scattered all over too.

I think going forward cut/paste. I'll keep them in separate commits so it's easy to track.

I'll probably take a couple of days to review the changes and test them