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

Fixed opt/get Int/Long/Double inconsistencies #661

Closed
wants to merge 7 commits into from

Conversation

abimarank
Copy link

As discussed here, this PR fixed the inconsistencies between get/opt Numbers

Copy link
Contributor

@johnjaylward johnjaylward left a comment

Choose a reason for hiding this comment

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

This looks good to me. I think removal of the hexfloat is probably OK. It is not a standard in JSON anyway.

@stleary
Copy link
Owner

stleary commented Feb 3, 2022

Possible problem with the unit tests?

@abimarank
Copy link
Author

abimarank commented Feb 4, 2022

@stleary The test failures aren't from these changes, they must be there earlier too.

For example, consider the following test case

@Test
public void verifyConstructor() {
    
    final JSONArray expected = new JSONArray("[10]");
    
    @SuppressWarnings("rawtypes")
    Collection myRawC = Collections.singleton("10");
    JSONArray jaRaw = new JSONArray(myRawC);

    Collection<Integer> myCInt = Collections.singleton(Integer.valueOf(10));
    JSONArray jaInt = new JSONArray(myCInt);

    Collection<Object> myCObj = Collections.singleton((Object) 10);
    JSONArray jaObj = new JSONArray(myCObj);

    System.out.println(jaRaw.toString());

    assertTrue(
            "The RAW Collection should give me the same as the Typed Collection",
            expected.similar(jaRaw));
    assertTrue(
            "The RAW Collection should give me the same as the Typed Collection",
            expected.similar(jaInt));
    assertTrue(
            "The RAW Collection should give me the same as the Typed Collection",
            expected.similar(jaObj));
}

The line JSONArray expected = new JSONArray("[10]"); outputs "["10"]" not "[10]". Hence the test case is failed.

What should be the behaviour with numbers in String format when providing through the constructor?

@johnjaylward
Copy link
Contributor

Something must be incorrect with the changes in this PR. That test case is correct.

@abimarank
Copy link
Author

@johnjaylward Testcase is correct.

The line JSONArray expected = new JSONArray("[10]"); outputs "["10"]" not "[10]"

It's like instead of producing an array of int, it produces an array of String numeric literals. Any hint why this behaviour?

@abimarank
Copy link
Author

abimarank commented Feb 6, 2022

I have fixed the issue with validating the String for numeric literals.

Do we need to consider 23.45e-4 as a valid numeral?

@stleary
Copy link
Owner

stleary commented Feb 6, 2022

Modifying stringToNumber() may not be the best way to address this issue. The goal is for the get* and opt* methods to behave the same, while minimizing changes to existing behavior.

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