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

clean up misc from retro-review of #101 #105

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

clean up misc from retro-review of #101 #105

wants to merge 5 commits into from

Conversation

asmello
Copy link
Collaborator

@asmello asmello commented Feb 17, 2025

I changed my mind on removing serde as a default feature. It is a heavy dependency to bring by default, but it's easy enough to disable the default features, and I do see the "common" use case for jsonptr involving serde_json. Also I guess Diagnose makes sense when you consider that the error it converts isn't yet a Report, even though it implements Diagnostic. Perhaps it's Diagnostic that requires reconsidering.

Changes I'm pushing forward:

  • make diagnostic::Label::new private
  • add docs do validate_bytes
  • remove NoLeadingSlash struct
  • update delete.rs docs
  • deprecate ResolveError

Left for standalone PRs:

  • Migrate index::ParseIndexError and token::EncodingError to the new error system.
  • Seal Diagnose

@asmello asmello requested a review from chanced February 17, 2025 20:36
@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.9%. Comparing base (b302568) to head (f2677c8).

Additional details and impacted files
Files with missing lines Coverage Δ
src/delete.rs 99.4% <ø> (ø)
src/diagnostic.rs 74.4% <ø> (ø)
src/pointer.rs 96.0% <ø> (+0.4%) ⬆️
src/resolve.rs 93.6% <ø> (ø)

@asmello
Copy link
Collaborator Author

asmello commented Feb 17, 2025

This is technically breaking, as reported by the semver check, but nobody would be using the removed type because it's useless / not referenced anywhere.

@asmello asmello mentioned this pull request Feb 17, 2025
@chanced
Copy link
Owner

chanced commented Feb 25, 2025

make diagnostic::Label::new private

This one needs to be public in order for users to implement Diagnostic properly. Users can bring their own errors for Assign and Resolve but they have to implement Diagnostic.

remove NoLeadingSlash struct

Ugh, thank you. Sorry I left that in.

@asmello
Copy link
Collaborator Author

asmello commented Feb 26, 2025

This one needs to be public in order for users to implement Diagnostic properly. Users can bring their own errors for Assign and Resolve but they have to implement Diagnostic.

Ah, interesting, but do we expect users to implement Diagnostic themselves?

@chanced
Copy link
Owner

chanced commented Mar 2, 2025

I dont necessarily expect it - I doubt anyone is implementing Assign and Resolve for custom languages or data structures - but it is possible and in an effort to facilitate that, the traits were designed so that implementations bring their own error types for resolve::Resolve and assign::Assign.

@asmello
Copy link
Collaborator Author

asmello commented Mar 2, 2025

You know what, you're completely right. I lost track of what those traits were for, it does make sense that people can use jsonptr with their own types. Although, now that I think about it, it also only makes sense for JSON-like data structures.

I'll update this PR and remove that change. Thanks!

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