-
Notifications
You must be signed in to change notification settings - Fork 5
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
|
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. |
This one needs to be public in order for users to implement
Ugh, thank you. Sorry I left that in. |
Ah, interesting, but do we expect users to implement |
I dont necessarily expect it - I doubt anyone is implementing |
You know what, you're completely right. I lost track of what those traits were for, it does make sense that people can use I'll update this PR and remove that change. Thanks! |
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 guessDiagnose
makes sense when you consider that the error it converts isn't yet aReport
, even though it implementsDiagnostic
. Perhaps it'sDiagnostic
that requires reconsidering.Changes I'm pushing forward:
diagnostic::Label::new
privatevalidate_bytes
NoLeadingSlash
structdelete.rs
docsResolveError
Left for standalone PRs:
index::ParseIndexError
andtoken::EncodingError
to the new error system.Diagnose