Skip to content

Introduce OmitOperation #1

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

Closed
wants to merge 8 commits into from
Closed

Conversation

hnguyen08
Copy link

No description provided.

hnguyen08 and others added 6 commits September 2, 2015 20:41
- Reintroduce old JsonPatch.fromJson API
- RegistryBasedJsonPatchFactory
- Don't default operations upon construction of RegistryBasedJsonPatchFactoryBuilder
- Rename RFC6902* -> Standard*
- Other minor changes
…ended functionality:

- Fixed bug where defaultFactory wasn't initialized
- Added extendedFactory
- Tests for the above
@@ -123,11 +123,9 @@ public JsonPatch(final List<JsonPatchOperation> operations)
* @throws NullPointerException input is null
*/
public static JsonPatch fromJson(final JsonNode node)
throws IOException
{
throws IOException, JsonPatchException {

Choose a reason for hiding this comment

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

this smells, usually you have one exception type coming out and either wrap the IOException or don't return a more specifically typed one.

Choose a reason for hiding this comment

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

Also JsonProcessingException?

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good. JsonProcessingException extends IOException, so that will be handled as well.

- Created *OperationFactory classes
- Minor style changes
- made some things final
* The JsonPatchOperationFactoryBase implements a few common operations
* for all JsonPatchOperationFactorys.
*/
public abstract class JsonPatchOperationFactoryBase implements JsonPatchOperationFactory

Choose a reason for hiding this comment

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

I'd be tempted to do this as a Helper instead of a Base.

@huggsboson
Copy link

Looks mostly good to me. Anyway you can fix the JsonPatchOperation interface thing and then get this patchset rebased on top of head (to eliminate my grails changes) so I can do a quick scan through?

@hnguyen08
Copy link
Author

Pushed final review changes. Going to rebase and squash (maybe push it to a different PR rather than force push so we keep this history)

@hnguyen08
Copy link
Author

Merged change up here: #2

@hnguyen08 hnguyen08 closed this Sep 23, 2015
@hnguyen08 hnguyen08 deleted the omit branch September 23, 2015 15:29
jasonchaffee added a commit to jasonchaffee/json-patch that referenced this pull request Jan 4, 2017
Adding a public accessor to JsonPatch to retrieve the contained operation and path
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