Skip to content

Allow custom JsonNode implementations #3701

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

Merged
merged 4 commits into from
Dec 20, 2022

Conversation

marschall
Copy link
Contributor

Fixes #3699

}

// [databind#3699]: custom array node classes
public void testCustomArrayNode() throws Exception
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was already working before, this test just verifies it.

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 17, 2022

Ok, change itself is fine. One thing on test: it'd be great to have something bit more compact if possible, looks like there's a lot of boilerplate copied code. I assume this is due to this being a real use case (one where delegation is needed); but since test really does not exercise much of it a smaller subset might work?

That is: could you just trim custom impls down to minimum needed for test. Not delegating all calls.
And with that, can (and should) embed them as static classes in test class itself to keep things nice & tidy.

@marschall
Copy link
Contributor Author

Ok, change itself is fine. One thing on test: it'd be great to have something bit more compact if possible, looks like there's a lot of boilerplate copied code. I assume this is due to this being a real use case (one where delegation is needed); but since test really does not exercise much of it a smaller subset might work?

That is: could you just trim custom impls down to minimum needed for test. Not delegating all calls. And with that, can (and should) embed them as static classes in test class itself to keep things nice & tidy.

Even when extending from BaseJsonNode the JsonNode API is quite large and a lot of methods have to be implemented just to get the code to compile. I changed many methods to instead be empty or return a hard coded result and made them inner classes.

@pjfanning pjfanning dismissed their stale review December 20, 2022 13:43

Changes made

@cowtowncoder
Copy link
Member

Even when extending from BaseJsonNode the JsonNode API is quite large and a lot of methods have to be implemented just to get the code to compile. I changed many methods to instead be empty or return a hard coded result and made them inner classes.

Ah ok. Thank you for explanation that makes sense.

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.

3 participants