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

feat: add from/to json for uint/uint64 #1417

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

peter-jerry-ye
Copy link
Collaborator

@peter-jerry-ye peter-jerry-ye commented Jan 3, 2025

For FromJson and ToJson, we only guarantee the roundtrip property.

So for UInt64 and Int64, we'll be using String as representation, and for UInt and Int, we'll be using Number as representation.

Copy link

peter-jerry-ye-code-review bot commented Jan 3, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Here are three observations from the provided git diff output:

  1. Inconsistent JSON Representation for Int64 and UInt64:

    • The Int64::to_json and UInt64::to_json methods now convert the values to strings (String::to_json(self.to_string())), whereas previously they were converted to Number. This change might break existing code that expects Int64 and UInt64 values to be serialized as Number in JSON. This inconsistency should be documented clearly, and any dependent code should be updated accordingly.
  2. Potential Overflow Issue in UInt::from_json:

    • In the UInt::from_json implementation, there is no explicit handling for overflow when converting a Number to UInt. For example, if the Number value is larger than the maximum value that UInt can hold, it might result in unexpected behavior. Consider adding checks or documentation to handle such cases.
  3. Copyright Year Update:

    • The copyright year has been updated from 2024 to 2025 in multiple files. While this is a minor change, ensure that this update is intentional and consistent across all relevant files in the project. If this change is not intentional, it should be reverted to avoid confusion.

These are the key points to consider based on the provided diff. Let me know if you need further clarification or assistance!

@coveralls
Copy link
Collaborator

coveralls commented Jan 3, 2025

Pull Request Test Coverage Report for Build 4691

Details

  • 7 of 17 (41.18%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.06%) to 82.928%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/json.mbt 0 3 0.0%
json/from_json.mbt 7 14 50.0%
Totals Coverage Status
Change from base Build 4690: -0.06%
Covered Lines: 4770
Relevant Lines: 5752

💛 - Coveralls

@peter-jerry-ye peter-jerry-ye force-pushed the zihang/from-to-json-uint branch from 4e8f0ae to 1eb07d4 Compare January 3, 2025 03:44
@peter-jerry-ye
Copy link
Collaborator Author

Waiting for #1419

@peter-jerry-ye peter-jerry-ye force-pushed the zihang/from-to-json-uint branch 2 times, most recently from 6555fa3 to d8aaedb Compare January 8, 2025 09:20
@peter-jerry-ye peter-jerry-ye marked this pull request as ready for review January 8, 2025 09:23
@peter-jerry-ye peter-jerry-ye force-pushed the zihang/from-to-json-uint branch from d8aaedb to 7e50776 Compare January 10, 2025 05:43
@bobzhang bobzhang force-pushed the zihang/from-to-json-uint branch from 7e50776 to a3b7c0b Compare January 10, 2025 08:38
@bobzhang bobzhang force-pushed the zihang/from-to-json-uint branch from a3b7c0b to ceb45b6 Compare January 10, 2025 08:38
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.

2 participants