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

Complete implementation of JSON Pointer and better errors. Fixes #94. #95

Merged
merged 21 commits into from
Mar 16, 2019

Conversation

holograph
Copy link
Collaborator

Yup, this is a pretty big change, following up on @LouizFC's work on #90. It fixes #94 and generally clears up a lot of crufty code, although it doesn't yet add the compatibility flag discussed in #90.

What started as a small fix ended up being a comprehensive refactoring, so I apologize in advance for the size of this PR. A few important notes for reviewers:

  • Most of the path parsing, rendering and navigation features are now in the JsonPointer class, so I suggest starting there and/or JsonPointerTest;
  • All existing tests still run with few or no modifications, so I'm pretty confident about this :-)

Let me know if you need any help, or alternatively have some ideas on how to break this down.

@@ -11,10 +11,10 @@
"node": { "x": "a" }
},
{
"op": [{ "op": "replace", "path": "non-existing-path", "value": "some-value"}],
"op": [{ "op": "replace", "path": "/non-existing-path", "value": "some-value"}],
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path in the original test was invalid per RFC 6901

@LouizFC
Copy link
Collaborator

LouizFC commented Feb 13, 2019

First of all, you did a trully wonderful job there. The code looks a lot more cleaner and the "Path" logic is all centralized now.

I took a quick look and everything looks fine, I will review it more carefully now, this is quite a big change indeed, so I think it will take a while to review it all, but I can do it before the weekend.

Copy link
Collaborator

@LouizFC LouizFC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that it would be a long review, but most changes in InPlaceProcessor were due to git messing up the diffs.

I want to test JsonPointer a bit before approving everything, but overall I think the PR looks fine

Copy link
Collaborator

@LouizFC LouizFC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more nitpicks

Copy link
Collaborator

@LouizFC LouizFC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. I will approve it here, everything looks fine and the StringTokenizer change is kinda of optional.
Thank you for this PR

@holograph
Copy link
Collaborator Author

Well I went back to the parser and looked at StringTokenizer. Straight out the gate, from the JavaDocs:

/** ...
 * {@code StringTokenizer} is a legacy class that is retained for
 * compatibility reasons although its use is discouraged in new code. It is
 * recommended that anyone seeking this functionality use the {@code split}
 * method of {@code String} or the java.util.regex package instead.
 */

If anyone wants to rewrite this in what they consider a simpler fashion, go for it. This isn't a really good parser anyway, there's absolutely no validation (Unicode character ranges etc.).

@LouizFC
Copy link
Collaborator

LouizFC commented Feb 14, 2019

I am really really sorry @holograph , I meant StringBuilder

@holograph
Copy link
Collaborator Author

Ah, that amounts to writing my own parser, which I'm too lazy to do without good reason :-)

@LouizFC
Copy link
Collaborator

LouizFC commented Feb 14, 2019

I too came back to write the parser and I got a question:

The current implementation parses any "wrong" input as a empty/Root JsonPointer, by wrong I mean inputs without / as a prefix (e.g. test/pointer). Is this intended?

@holograph
Copy link
Collaborator Author

That's what I get for being lazy. Fixed.

@vishwakarma Ball's in your court :-)

@vishwakarma vishwakarma merged commit 39b8637 into flipkart-incubator:master Mar 16, 2019
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.

Improve error messages (especially for test ops)
3 participants