Skip to content

Conversation

GnaneshKunal
Copy link
Contributor

@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
Contributor

@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
Contributor Author

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

Copy link
Contributor

@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
Contributor 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
Contributor

@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 AmirAllayarovSofteq self-requested a review May 21, 2024 06:59
@AmirAllayarovSofteq AmirAllayarovSofteq self-requested a review May 21, 2024 07:14
@AmirAllayarovSofteq
Copy link
Contributor

@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
Contributor Author

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

image

I have extended the support to Vector types as well:

image

Copy link
Contributor

@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
Contributor Author

@AmirAllayarovSofteq,

I have added the tests.

Copy link
Contributor

@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
@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.

3 participants