chanced / jsonptr

JSON Pointer (RFC 6901) implementation for Rust
Apache License 2.0
44 stars 4 forks source link

Refactor constructor safety #76

Closed asmello closed 2 weeks ago

asmello commented 3 weeks ago

Although a lot more inconvenient for us, some of our functions have invariants that have to be held externally, so they should be unsafe fn. This also makes it justifiable to expose them publicly (as then users can opt-in for manual checking of the invariants).

Notable changes:

codecov-commenter commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.0%. Comparing base (c96007e) to head (aeb33b1).

Additional details and impacted files | [Files with missing lines](https://app.codecov.io/gh/chanced/jsonptr/pull/76?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance) | Coverage Δ | | |---|---|---| | [src/pointer.rs](https://app.codecov.io/gh/chanced/jsonptr/pull/76?src=pr&el=tree&filepath=src%2Fpointer.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance#diff-c3JjL3BvaW50ZXIucnM=) | `98.1% <100.0%> (+<0.1%)` | :arrow_up: | | [src/token.rs](https://app.codecov.io/gh/chanced/jsonptr/pull/76?src=pr&el=tree&filepath=src%2Ftoken.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance#diff-c3JjL3Rva2VuLnJz) | `100.0% <100.0%> (ø)` | |
asmello commented 3 weeks ago

I think the sanitizer test is failing due to a bug in the sanitizer itself (given the obscure error referencing its source code).

My guess is https://github.com/rust-lang/rust/issues/111073

chanced commented 2 weeks ago

I'm at the fair with my family but noticed this come through and just wanted to say that I'm stoked about this one. I considered suggesting exposing Pointer::new but was hesitant.

I'll review both tomorrow morning.

Also, you're awesome dude.

asmello commented 2 weeks ago

No rush, just came with a few ideas recently.

I noticed json-patch recently upgraded to 0.6.0, which is great (and I feel bad I let them do it, meant to contribute a PR myself).

chanced commented 2 weeks ago

Hah, you did! Perhaps not to hatch directly but the state of this crate would be in much worse shape without your contributions. You brought it up a few levels! :)

asmello commented 2 weeks ago

To be honest the unsafe for internal uses might be a bit overkill, I'm not 100% sure this is a great idea long term, but since it's an internal detail I think it's ok to experiment with.