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

RI-5523: Fallback to non-native big integers for JSON formatting #3231

Merged
merged 7 commits into from
Jun 6, 2024

Conversation

GnaneshKunal
Copy link
Collaborator

@GnaneshKunal GnaneshKunal self-assigned this Apr 6, 2024
@GnaneshKunal GnaneshKunal changed the title RI-5523: Fallback to non-native big integers in JSON RI-5523: Fallback to non-native big integers in JSON viewer Apr 6, 2024
@GnaneshKunal GnaneshKunal changed the title RI-5523: Fallback to non-native big integers in JSON viewer RI-5523: Fallback to non-native big integers for JSON formatting Apr 6, 2024
Copy link
Collaborator

@AmirAllayarovSofteq AmirAllayarovSofteq left a comment

Choose a reason for hiding this comment

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

Screenshot from 2024-04-09 14-41-19
Need to fix. (12345678919232131 value in this key)

@GnaneshKunal GnaneshKunal force-pushed the bugfix/RI-5523-allow-json-non-native-big-int branch from c2eb8f9 to 75d1b2e Compare May 15, 2024 07:09
@GnaneshKunal
Copy link
Collaborator Author

@AmirAllayarovSofteq, Can you approve the CI job and verify it?

Copy link
Collaborator

@AmirAllayarovSofteq AmirAllayarovSofteq left a comment

Choose a reason for hiding this comment

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

May be I'm doing something wrong. But from example :
Screenshot from 2024-05-20 12-18-46
Screenshot from 2024-05-20 12-18-58
So i guess problem doesn't fixed

@GnaneshKunal
Copy link
Collaborator Author

@AmirAllayarovSofteq

I have pointed this out in the ticket. According to the JSON specification, only big numerics are supported, not big floating-point numbers. The only way to view this is to use scientific notation (which is usually the use case for big numbers). Our vector format mostly has big floating-point numbers that use scientific notation only.

@ViktarStarastsenka Maybe we can show a chip denoting that the number has been converted to scientific notation?

@AmirAllayarovSofteq
Copy link
Collaborator

@GnaneshKunal Sorry, my bad.
@ViktarStarastsenka So if it is okay for you, I'm fine with it. But in "edit" mode user will see another value,

@AmirAllayarovSofteq
Copy link
Collaborator

@GnaneshKunal
1 - We can improve JsonPretty. const data = JSONBigInt({ useNativeBigInt }).parse(value) will return scientific notation fro huge numbers or numbers with huge float part. We can handle scientific notation. This value will have own _isBigNumber property. So we can handle those values in JsonPretty component.
And update JsonPrimitive. Scientific notation has toString()

2 - Probably in JSONViewer we should add something like this:
try { const jsonValue = RenderJSONValue(value, expanded, space, useNativeBigInt) return { value: jsonValue, isValid: true } } catch (e) { try { const jsonValue = RenderJSONValue(value, expanded, space, false) return { value: jsonValue, isValid: true } } catch (e) { return { value, isValid: false } } }
We will cover both cases. When values will have native BigInt or will not have. And can parse both. I'm not sure here. Up to you

@GnaneshKunal
Copy link
Collaborator Author

Done. Thanks @AmirAllayarovSofteq. That was an excellent suggestion.

image

I have extended the support to Vector types as well:

image

Copy link
Collaborator

@AmirAllayarovSofteq AmirAllayarovSofteq left a comment

Choose a reason for hiding this comment

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

LGTM! Maybe you can add one test for _isBigNumer in JsonPretty

@GnaneshKunal
Copy link
Collaborator Author

@AmirAllayarovSofteq,

I have added the tests.

Copy link
Collaborator

@AmirAllayarovSofteq AmirAllayarovSofteq left a comment

Choose a reason for hiding this comment

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

Good job!

@vlad-dargel vlad-dargel merged commit ff50970 into main Jun 6, 2024
25 of 27 checks passed
@vlad-dargel vlad-dargel deleted the bugfix/RI-5523-allow-json-non-native-big-int branch June 6, 2024 14:03
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

3 participants