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

Small DX changes + more docs #61

Merged
merged 12 commits into from
Jul 9, 2024
Merged

Small DX changes + more docs #61

merged 12 commits into from
Jul 9, 2024

Conversation

chanced
Copy link
Owner

@chanced chanced commented Jul 9, 2024

  • renames PointerBuf::replace_token to replace, ReplaceTokenError to ReplaceError
  • changes PointerBuf::push_back, PointerBuf::push_front, PointerBuf::replace to accept T: Into<Token>
  • implements From all integer types for Token
  • cleans up and adds docs

@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.1%. Comparing base (02e1b95) to head (5e6097b).
Report is 5 commits behind head on main.

Additional details and impacted files
Files with missing lines Coverage Δ
src/index.rs 100.0% <ø> (ø)
src/pointer.rs 98.1% <100.0%> (+<0.1%) ⬆️
src/token.rs 100.0% <100.0%> (ø)

@chanced
Copy link
Owner Author

chanced commented Jul 9, 2024

@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
Copy link
Owner Author

chanced commented Jul 9, 2024

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.

@chanced chanced marked this pull request as draft July 9, 2024 18:10
@chanced chanced marked this pull request as ready for review July 9, 2024 18:35
@asmello
Copy link
Collaborator

asmello commented Jul 9, 2024

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

chanced commented Jul 9, 2024 via email

@asmello
Copy link
Collaborator

asmello commented Jul 9, 2024

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

@chanced
Copy link
Owner Author

chanced commented Jul 9, 2024

@asmello i went ahead and removed serde from Token.

@asmello
Copy link
Collaborator

asmello commented Jul 9, 2024

@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. 👍

@chanced chanced merged commit bf73713 into main Jul 9, 2024
20 checks passed
@chanced chanced deleted the more-docs branch July 9, 2024 21:43
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