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

Add "JsonPointer#appendProperty" and "JsonPointer#appendIndex" #684

Closed
LouizFC opened this issue Mar 10, 2021 · 3 comments · Fixed by #722
Closed

Add "JsonPointer#appendProperty" and "JsonPointer#appendIndex" #684

LouizFC opened this issue Mar 10, 2021 · 3 comments · Fixed by #722
Labels
2.14 Issue planned (at earliest) for 2.14
Milestone

Comments

@LouizFC
Copy link

LouizFC commented Mar 10, 2021

When working heavily with JsonPointers, the following patterns usually emerges:

JsonPointer root = JsonPointer.compile("/a/b");
/// ... code
String property = "c"
root.append(JsonPointer.compile("/" + property));
/// or
JsonPointer.compile(root + "/" + property)

This is quite verbose, also a bit error prone, because it is easy to forget to add the / before each append.

Also, I found no easy way of appending slash characters without prior escaping.

So the two methods proposed should help with both convenience and some edge cases. The only thing that worries me is overusing/misusing this methods. Jackson JsonPointer is head centric, appending to the tail is expensive, so maybe documenting a warning should be good?

I will open a PR when I get some free time. I did a similar thing in the past, so I think I can probably reuse the tests from there.

@cowtowncoder
Copy link
Member

Agreed with all of above. If users need appending versions it makes sense to have a safe way to do that -- but also make them aware of efficiency aspects.

A note on complexity/overhead would be useful since as you point out, tail-append is much costlier than head-append (but alas, users mostly need tail one :) ).

@ILGO0413
Copy link
Contributor

Not sure if the warning is useful though, as head-append is protected and not provided as public API method. The main method to generate JsonPointer is JsonPointer#compile with full path expression, and it is used for both tail-append and head-append.
However, warning is added for future updates and internal API usage.

@cowtowncoder cowtowncoder added 2.14 Issue planned (at earliest) for 2.14 and removed 2.13 labels Oct 19, 2021
@cowtowncoder cowtowncoder added this to the 2.14.0 milestone Oct 19, 2021
cowtowncoder added a commit that referenced this issue Oct 19, 2021
@cowtowncoder
Copy link
Member

Merged, will be in 2.14.0: thank you @ILGO0413 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.14 Issue planned (at earliest) for 2.14
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants