jsonptr icon indicating copy to clipboard operation
jsonptr copied to clipboard

clean up misc from retro-review of #101

Open asmello opened this issue 1 year ago • 6 comments

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 avatar Feb 17 '25 20:02 asmello

Codecov Report

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

Project coverage is 95.9%. Comparing base (29840e7) to head (865aee2). Report is 1 commits behind head on main.

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

... and 1 file with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov-commenter avatar Feb 17 '25 20:02 codecov-commenter

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 avatar Feb 17 '25 20:02 asmello

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.

chanced avatar Feb 25 '25 19:02 chanced

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?

asmello avatar Feb 26 '25 07:02 asmello

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.

chanced avatar Mar 02 '25 00:03 chanced

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!

asmello avatar Mar 02 '25 12:03 asmello

it also only makes sense for JSON-like data structures.

So YAML's integer based maps were what I had in mind when I created it. I didn't look to see whether or not serde_yaml supported the structure but figured if a crate ever came along that did, they'd need their own error.

In retrospect, it was likely overkill and have made the errors themselves a bigger pain than needed.

chanced avatar Jun 03 '25 12:06 chanced

Since you haven't explicitly nacked, and other than the deprecation this is mostly docs changes, I'm going to proceed with merging. 👍

asmello avatar Jun 08 '25 18:06 asmello