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: implement (From|To)Json for more @immut/*.T types #1136

Merged
merged 7 commits into from
Oct 18, 2024

Conversation

rami3l
Copy link
Contributor

@rami3l rami3l commented Oct 17, 2024

This pull request introduces JSON (de)serialization functionality to various immutable data structures (list, sorted_map, and sorted_set). The changes include implementing the (From|To)Json traits for these structures, adding corresponding methods, and updating the test cases to validate the new functionality.

As previously discussed with @bobzhang.

Copy link

peter-jerry-ye-code-review bot commented Oct 17, 2024

The provided git diff output reveals several changes and additions across multiple files within a project, particularly focusing on adding JSON serialization and deserialization capabilities for various data structures. Here are three key observations and suggestions based on the changes:

  1. Potential Bug in JSON Deserialization for Sorted Set (immut/sorted_set/types.mbt):

    • The line +} derive(Default) suggests that the Default trait is derived for the T enum, which represents a sorted set. However, the commit message does not indicate why Eq was removed. Deriving Default for an enum typically requires a default variant (e.g., Empty), but it's crucial to ensure that all operations involving this type correctly handle the default case, especially in deserialization logic where incomplete data might lead to unexpected behavior.
    • Suggestion: Review the deserialization logic to ensure it properly handles the Default case and verify why Eq was removed. If Eq is still needed, consider re-adding it.
  2. Potential Typo in JSON Deserialization Error Message (immut/list/list.mbt):

    • The error message "@immut/list.from_json: expected array" seems correct, but it's always good to double-check error messages for typos or unclear language.
    • Suggestion: Ensure the error message is clear and concise. If possible, include more context or the expected JSON structure to aid debugging.
  3. Incomplete Documentation for New JSON Functions:

    • Several new functions for JSON serialization (to_json) and deserialization (from_json) are added across multiple files, but the commit does not include updates to the documentation comments for these new functions.
    • Suggestion: Update the doc comments for each new to_json and from_json function, providing examples and explaining the expected input and output formats. This will greatly assist users in understanding how to use these functions correctly.

These observations focus on potential issues that could affect the robustness and usability of the code, particularly around the new JSON handling capabilities. Addressing these points will help ensure the code is both reliable and user-friendly.

@rami3l rami3l force-pushed the feat/more-to-json branch 2 times, most recently from 2569ddd to 029238c Compare October 17, 2024 03:38
@coveralls
Copy link
Collaborator

coveralls commented Oct 17, 2024

Pull Request Test Coverage Report for Build 3625

Details

  • 37 of 44 (84.09%) changed or added relevant lines in 4 files are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.4%) to 82.688%

Changes Missing Coverage Covered Lines Changed/Added Lines %
immut/sorted_set/generic.mbt 12 13 92.31%
immut/list/list.mbt 9 11 81.82%
immut/sorted_map/utils.mbt 8 10 80.0%
immut/sorted_set/immutable_set.mbt 8 10 80.0%
Files with Coverage Reduction New Missed Lines %
immut/sorted_set/immutable_set.mbt 5 89.42%
Totals Coverage Status
Change from base Build 3616: 0.4%
Covered Lines: 4251
Relevant Lines: 5141

💛 - Coveralls

@rami3l rami3l force-pushed the feat/more-to-json branch 2 times, most recently from 63dd646 to 5edeef8 Compare October 17, 2024 04:44
@rami3l rami3l force-pushed the feat/more-to-json branch 2 times, most recently from a37e6cb to 7141896 Compare October 17, 2024 09:23
@rami3l rami3l changed the title feat: implement ToJson for more @immut/*.T types feat: implement (From|To)Json for more @immut/*.T types Oct 17, 2024
@bobzhang bobzhang merged commit d1ab024 into moonbitlang:main Oct 18, 2024
11 of 13 checks passed
@rami3l rami3l deleted the feat/more-to-json branch October 18, 2024 02:47
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.

5 participants