JSON Pointer implementation - #222
Conversation
|
This looks like a nice simple implementation to me. I have not looked at the test cases to see if they cover the different pointer types available. @erosb can you also update the README to indicate which RFC this is implementing? |
|
@johnjaylward I've just added it in cbb1546 |
|
What problem does this code solve? Changes to the API?
2 new classes are also defined:
Does it break the unit tests? Will this require a new release? Should the documentation be updated? |
|
I finally got around to looking at the test cases and saw this: assertSame(document.get("c%d"), query("#/c%25d"));
assertSame(document.get("k\"l"), query("/k\\\\\\\"l"));I was wondering if it made sense to provider a "builder" where you can add parts. Something like this maybe: JSONPointerBuilder jpb = new JSONPointerBuilder(); // will return the whole document
jpb.append("c%d");the So the example from the RFC JSONPointer jp = new JSONPointerBuilder().append("foo").append(0).compile();
// this would compile to "/foo/0"maybe this would be better in a separate pull request? I was also thinking that being able to get the path back from a JsonPointer may be useful jp.toString(); // should return the normalized non-URI path "/c%d"
jp.toURIPath(); // should return the uri encoded path "#/c%25d" |
|
@johnjaylward @stleary I've just updated the PR according to the above suggestions. |
| } | ||
| } | ||
|
|
||
| public JSONPointer(List<String> refTokens) { |
There was a problem hiding this comment.
This should either be a protected/private/or default constructor, and the List should be copied so the Pointer can't be modified outside of the class.
this.refTokens = refTokens.clone();as it is now, I could do this:
Builder b = JSONPointer.builder().append("key1");
JSONPointer jp1 = b.build();
b.append("key2");
JSONPointer jp2 = b.build();
if(jp1.toString().equals(jp2.toString()){
throw new Exception("Oops, my pointers are sharing a backing array");
}|
@johnjaylward good point, I've just added token list copying to the constructor. I kept it |
|
@stleary will you merge it in the next release? |
|
@erosb thanks for being patient. No review comments or code changes in over a week. No cautions about adding code that is not central to the purpose of the app. Still need to come up with some content for the readme. Will do a final code review and test run, then should be ready for the merge. Should be able to get to this over the weekend. |
See #218 for details