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

Represent serialized floats to python float precision #164

Merged
merged 4 commits into from Apr 2, 2022

Conversation

bennyweise
Copy link
Contributor

Currently, the general string format used in the serialisation of a float value uses the default precision of 6, which leads to a loss in precision of large/long numbers. This PR changes the precision to use up to 16 places, which is approximately the level of precision of a float. This doesn't have any impact on the existing tests that don't have numbers with precision > 6, and the added assertion in the existing test fails without the change.

@bennyweise bennyweise requested a review from Cito as a code owner April 1, 2022 03:42
Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR! Please check my comments below.

src/graphql/utilities/ast_from_value.py Outdated Show resolved Hide resolved
tests/utilities/test_ast_from_value.py Show resolved Hide resolved
@bennyweise bennyweise requested a review from Cito April 2, 2022 08:48
Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

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

Looks good. Can you change the IntValueNode case to use str as well? Should make no difference, but the code will look more consistent then.

tests/utilities/test_ast_from_value.py Outdated Show resolved Hide resolved
src/graphql/utilities/ast_from_value.py Outdated Show resolved Hide resolved
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.

None yet

2 participants