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

Only coerce scalars if ALLOW_COERCION_OF_SCALARS is enabled #70

Merged
merged 2 commits into from
Jan 28, 2019

Conversation

dansanduleac
Copy link
Contributor

@dansanduleac dansanduleac commented Jan 9, 2019

Fixes #69

This fix is for 2.9, but it should merge without conflicts into 2.10 as well.

@cowtowncoder
Copy link
Member

cowtowncoder commented Jan 10, 2019

Sounds reasonable; thank you for contributing this fix!

One request: would it be possible to add a matching unit test(s)? jackson-databind has a few tests to copy-paste, possibly:

src/test/java/com/fasterxml/jackson/databind/struct/ScalarCoercionTest.java

and it would be great to check against regression, as Afterburner's handling is quite fragile due to low-level nature of optimization.

@dansanduleac
Copy link
Contributor Author

@cowtowncoder do you think this is OK to merge now?

@cowtowncoder
Copy link
Member

@dansanduleac Yes, I'd be happy to merge this. Just one more thing: unless I have asked for it (for any Jackson project), we need CLA. It can be found from:

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

and usual way is to print it, fill & sign, scan, email. Normally I'd ask email to info at fasterxml dot com BUT unfortunately we are having some issues with that so if you can make it shareable some other way (can one attach document to a gist?), that'd be great.

As soon as I get that I'll go ahead and merge the patch. Once again thank you for submitting this & apologies for the hassle wrt cla.

@dansanduleac
Copy link
Contributor Author

@cowtowncoder not sure if I can add it to a gist, but if you give me some other e-mail address I can send it to, I'd be happy to!

@cowtowncoder
Copy link
Member

@dansanduleac Actually info at fasterxml dot com works again, so use that.

@cowtowncoder cowtowncoder merged commit 593c883 into FasterXML:2.9 Jan 28, 2019
@cowtowncoder
Copy link
Member

CLA received, filed, merged the fix. Thank you! This will be in 2.9.9 (and 2.10.0+)

@dansanduleac dansanduleac deleted the ds/fix-coerce-afterburner branch January 28, 2019 16:19
bulldozer-bot bot pushed a commit to palantir/conjure-java-runtime that referenced this pull request Mar 12, 2019
## Before this PR

Ignore file was mentioning the wrong jackson flag - we already use ALLOW_COERCION_OF_SCALARS, but it doesn't work with afterburner. See FasterXML/jackson-modules-base#70

## After this PR

Fixed comments to more accurately describe the ignores that will be fixed by jackson 2.9.9
stevenschlansker pushed a commit to stevenschlansker/jackson-modules-base that referenced this pull request Sep 5, 2019
…burner

Only coerce scalars if ALLOW_COERCION_OF_SCALARS is enabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants