chanced / jsonptr

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

Small DX changes + more docs #61

Closed chanced closed 1 month ago

chanced commented 1 month ago
codecov-commenter commented 1 month ago

Codecov Report

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

Project coverage is 99.1%. Comparing base (02e1b95) to head (5e6097b).

Additional details and impacted files | [Files](https://app.codecov.io/gh/chanced/jsonptr/pull/61?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance) | Coverage Δ | | |---|---|---| | [src/index.rs](https://app.codecov.io/gh/chanced/jsonptr/pull/61?src=pr&el=tree&filepath=src%2Findex.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance#diff-c3JjL2luZGV4LnJz) | `100.0% <ø> (ø)` | | | [src/pointer.rs](https://app.codecov.io/gh/chanced/jsonptr/pull/61?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/61?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%> (ø)` | |
chanced commented 1 month ago

@asmello any objection to renaming replace_token to replace, ReplaceTokenError to ReplaceError? All other methods omit token so I figured it'd align a bit better. The only hesitation I have is that replace for string types may give a slightly different expectation.

Also, do you have any opposition to me implementing PartialEq<&str> for Token (using the decoded value)?

edit: motivation for the PartialEq was for this doc example:

let ptr = Pointer::from_static("/path/to/value");
let tokens: Vec<_> = ptr.tokens().collect();
assert_eq!(tokens, vec!["path", "to", "value"]);

vs

let ptr = Pointer::from_static("/path/to/value");
let tokens: Vec<_> = ptr.tokens().collect();
assert_eq!(tokens, vec![Token::new("path"), Token::new("to"), Token::new("value")]);
chanced commented 1 month ago

This PR will be the second to last I want to finish.

The next will contain a small change to Component, adding offset as well as adding walk_to / walk_form to Resolve with provided implementations.

asmello commented 1 month ago

Also, do you have any opposition to me implementing PartialEq<&str> for Token (using the decoded value)?

I'm a bit on the fence on this one. While the improved ergonomics is nice, I'm a little afraid people won't realise that it uses the encoded version until they run into an unexpected edge case at runtime. Seems a bit error prone. By forcing straight Token and Token comparison there's no room for error.

I can change my mind on this, if it turns out to be a major pain point, but I'd err on the side of safety at first.

EDIT: forgot to say, happy with the renames, didn't notice the inconsistency 👍

chanced commented 1 month ago

Sounds good to me.

On Tue, Jul 9, 2024 at 2:44 PM André Mello @.***> wrote:

Also, do you have any opposition to me implementing PartialEq<&str> for Token (using the decoded value)?

I'm a bit on the fence on this one. While the improved ergonomics is nice, I'm a little afraid people won't realise that it uses the encoded version until they run into an unexpected edge case at runtime. Seems a bit error prone. By forcing straight Token and Token comparison there's no room for error.

I can change my mind on this, if it turns out to be a major pain point, but I'd err on the side of safety at first.

— Reply to this email directly, view it on GitHub https://github.com/chanced/jsonptr/pull/61#issuecomment-2218407189, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIGFZCVPGJLST2VZB7YNNDZLQVQJAVCNFSM6AAAAABKTH5CM6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJYGQYDOMJYHE . You are receiving this because you authored the thread.Message ID: @.***>

asmello commented 1 month ago

Also worth adding a comment so we remember we deliberately chose not to implement this.

chanced commented 1 month ago

@asmello i went ahead and removed serde from Token.

asmello commented 1 month ago

@asmello i went ahead and removed serde from Token.

Cool, I think that's reasonable. It's not immediately useful and there's ambiguity with encoding. 👍