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

POC API that avoids clone for consideration. #44

Merged
merged 5 commits into from
Jul 22, 2023
Merged

Conversation

BrynCooke
Copy link
Contributor

Hi!

I came across this library a while back when I was looking at different jsonpath libraries and it looks nice. The only issue seemed to be that the JsonPathFinder API seemed to require cloning to happen, which was a showstopper for my use case. Recently I had cause to look again as the other major jsonpath library for Rust looks abandoned and has bugs.

It looks like the underlying query implementation for jsonpath-rust supports query without having to clone the input data, however the use of JsonPathFinder forces this to happen.

This PR is a POC API for querying without cloning that introduces a new enum JsonPtr for holding values. The reason this has to exist is that deref on JsonPathValue is not possible without a panic for NoValue.

It'd be great if this approach could be considered for this project.

It looks like the underlying query implementation supports query without having to clone the input data, however the use of JsonPathFinder forces this to happen.

Added a POC API for querying without cloning that introduces a new enum `JsonPtr` for holding values. The reason this has to exist is that deref on JsonPathValue is not possible.
@besok
Copy link
Owner

besok commented Jul 20, 2023

Hi. Thanks for the PR. That is great, that you use the library.

Sure, the change seems to be useful. let's bring it in

@BrynCooke
Copy link
Contributor Author

Awesome. I've pushed some minor polish. Let me know if there's anything else you'd like to see. Of course if you have changes in mind that you'd like to make yourself then I'm also happy for you take it over the line.

@besok besok self-requested a review July 21, 2023 23:46
@besok besok merged commit 7df5619 into besok:main Jul 22, 2023
@besok
Copy link
Owner

besok commented Jul 22, 2023

Great. Thanks. That is a really helpful change.

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.

2 participants