Skip to content

Conversation

@geoand
Copy link
Contributor

@geoand geoand commented Aug 25, 2017

As one can clearly see from the changes included in the PR, ByteBuddy does all the heavy lifting that would otherwise is manually done with ASM, by utilizing it's declarative DSL.
This makes the code easier to read and (hopefully :)) easier to maintain.

Regarding the details of this particular PR, I opted to make as few changes as possible, leaving the structure of the package intact and simply replacing all the ASM parts with the respective ByteBuddy parts.

@geoand
Copy link
Contributor Author

geoand commented Aug 25, 2017

The second commit implements #2 and #3 but only for interfaces :)

import java.util.List;


final class TypeDefinitionUtil {
Copy link
Member

Choose a reason for hiding this comment

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

This is a good start, although I suspect it might run into trouble for nested generic types and/or due to Jackson's bit inconsistent definition of generics for Maps and Collections (for which it exposes generics used for binding Map / Collection and not to specific subtype).
But we can cross that bridge when get there since I doubt existing code handles these any better.

Copy link
Contributor Author

@geoand geoand Aug 25, 2017

Choose a reason for hiding this comment

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

Although I didn't run into any specific issues, I am sure that the are some obscure cases where the simple implementation I provided would stumble and fall

@cowtowncoder
Copy link
Member

Sounds good as far as it goes. Code does look cleaner, and addition of equals and hashCode is a great addition. The only thing I have to think about is versioning; ideally this would be done across minor versions and not in a patch. So perhaps this is something to merge for 3.0 and not 2.9(.1).

One practicality: unless we already have CLA for you we'd need one from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

(or corporate-CLA variant if preferable; most contributors use personal one)

and it's only needed for the first Jackson contribution, good for any number afterwards.

Usual way is to print, fill & sign, scan and email to info at fasterxml dot com.

Thank you again for this contribution.

@geoand
Copy link
Contributor Author

geoand commented Aug 25, 2017

Thank you for your support!

I just sent the required CLA agreement.

For what it's worth, I also believe that this change should be merged in 3.0 not 2.9.x.

Please let me know if I can do anything else to help :)

@cowtowncoder
Copy link
Member

Excellent! I may have to wait for just a little bit until changing master to be for 3.0, but with CLA should be able to merge as soon as I make that switch (most likely after 2.9.1 patch release).
Similar change would probably eventually make sense for Afterburner project as well.

@geoand
Copy link
Contributor Author

geoand commented Aug 26, 2017 via email

@geoand
Copy link
Contributor Author

geoand commented Sep 3, 2017

I rebased the PR in order to incorporate the bump the version 1.7.5 of ByteBuddy

@geoand
Copy link
Contributor Author

geoand commented Sep 3, 2017

For some odd reason it seems like TravisCI is not setting up the JDK 7 tests correctly resulting in the failure to run the tests using it (while JDK 8 is working correctly).

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 4, 2017

@geoand Yes I have noticed there's that JDK7 problem across projects. Haven't had time (nor experience with Travis or Jenkins) to dig into root cause. It used to work just fine until quite recently; just not sure if it's environmental (something changed in Travis) or something to do with parent poms of Jackson project.

@geoand
Copy link
Contributor Author

geoand commented Sep 5, 2017

I unfortunately don't have any TravisCI experience either :(

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