Skip to content

Fix #146: Underscores in numbers #203

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

Conversation

conor-ward
Copy link
Contributor

This PR fixes #146 for yaml. The test code below from that issue now passes but I'm unsure on where this test should exist.

    private static class LongHolder {
        private Long v;

        public Long getV() {
            return v;
        }

        public void setV(Long v) {
            this.v = v;
        }
    }

    private static class IntHolder {
        private Integer v;

        public Integer getV() {
            return v;
        }

        public void setV(Integer v) {
            this.v = v;
        }
    }

    @Test
    public void testYamlUnderscores() throws IOException {
        ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
        mapper.readValue("v: 1000000", IntHolder.class); // ok
        mapper.readValue("v: 1_000_000", IntHolder.class); // ok
        mapper.readValue("v: 1000000", LongHolder.class); // ok

        // throws: com.fasterxml.jackson.databind.JsonMappingException: For input string: "1_000_000"
        mapper.readValue("v: 1_000_000", LongHolder.class);
    }

For the fix, it looks like the original fix for #65 here missed using the _cleanedTextValue in the two places in the PR

@conor-ward conor-ward changed the title Issue 146: Underscores in numbers Fix #146: Underscores in numbers Jun 23, 2020
@kupci
Copy link
Member

kupci commented Jun 23, 2020

I'm unsure on where this test should exist.
What about StreamingParseTest (based on previous work done for this fix)?

public void /*test*/ IntParsingUnderscoresSm() throws Exception

@cowtowncoder
Copy link
Member

Yes, StreamingParseTest seems reasonable to me.

@cowtowncoder
Copy link
Member

@conor-ward Looks good; I can manually add the test method, or you can change PR; either way is fine.

But one thing we'll need (if not asked for it yet) is CLA. It can be found here:

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

and the usual way is to print it, fill & sign, scan/photo, email to info at fasterxml dot com.
This only needs to be done once, before the first contribution, and then it's good for any number of future contributions.

Thank you again for providing the fix!

@conor-ward
Copy link
Contributor Author

Took the test from failing directory on master here and put it in StreamingParseTest
Will send CLA on also

@cowtowncoder
Copy link
Member

Sounds good!

@cowtowncoder cowtowncoder merged commit 2cf9d0a into FasterXML:2.10 Jun 25, 2020
@cowtowncoder cowtowncoder added this to the 2.10.5 milestone Jun 25, 2020
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