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

Revise a need of JsonPathFinder and groom the API in general. #59

Open
besok opened this issue Dec 7, 2023 · 4 comments
Open

Revise a need of JsonPathFinder and groom the API in general. #59

besok opened this issue Dec 7, 2023 · 4 comments

Comments

@besok
Copy link
Owner

besok commented Dec 7, 2023

JsonPathFinder seems to be redundant. Probably, better to revise the API a remove it

@xMAC94x
Copy link
Contributor

xMAC94x commented Dec 11, 2023

@besok 3 potential API changes would also be:

  • get rid of JsonPathInst, just export the JsonPath (in case it changes in the future, we use semver anyways)
  • change jsonpath_rust::JsonPathQuery interface to use a JsonPath rather than a &str, but at the same time implement the From<&str> trait for JsonPath, so that json.path("$..book[?(@.author size 10)].title".into()) works
  • we have JsonPathValue and JsonPtr, maybe we only need one and have Option<JsonPtr> for the None case

@xMAC94x
Copy link
Contributor

xMAC94x commented Dec 11, 2023

I can have a look into these points after #58 got merged. Not a huge fan of gigantic refactoring PRs, rather do it in smaller steps. What do you think ?

@xMAC94x
Copy link
Contributor

xMAC94x commented Dec 12, 2023

@besok any comment at this ? maybe any way to make communication easier than via github ? Like discord ?

@besok
Copy link
Owner Author

besok commented Dec 12, 2023

Hey! Sorry, I am on vacation and very slowly respond. Yeah, sure. You can take it and split it into a smaller prs. I will finish the review of the current pr this week.

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

No branches or pull requests

2 participants