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

bring back optional field test #1067

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

Conversation

bobzhang
Copy link
Contributor

@bobzhang bobzhang commented Oct 8, 2024

No description provided.

Copy link

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

Observations and Suggestions

  1. Typo in Test Name:

    • In the json/to_json_test.mbt file, there is a commented-out test block with a typo in the test name. The correct term should be "optional" instead of "optinal".
    • Suggestion: Correct the typo in the test name and uncomment the test block to ensure it runs as expected.
  2. Incorrect Data Type Conversion:

    • In several functions within the bool/bool.mbt file, there are instances where the conversion from Bool to Int64, UInt, and UInt64 uses incorrect suffixes (L, U, UL). In MoonBit, these suffixes are not used; instead, the literals should be directly assigned.
    • Suggestion: Update the literals to remove the incorrect suffixes, e.g., change 1L to 1, 1U to 1, and 1UL to 1.
  3. Inconsistent JSON Representation:

    • In the builtin/json.mbt file, the Bool::to_json function converts Bool values to Json using True and False. However, in MoonBit, the correct constants for boolean JSON values are true and false.
    • Suggestion: Update the Bool::to_json function to use true and false instead of True and False.

These observations focus on potential bugs and inconsistencies that could affect the correctness and readability of the code. Addressing these issues will help ensure that the code is both functional and adherent to MoonBit's syntax and conventions.

@coveralls
Copy link
Collaborator

coveralls commented Oct 8, 2024

Pull Request Test Coverage Report for Build 3335

Details

  • 6 of 8 (75.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 83.283%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/json.mbt 0 2 0.0%
Totals Coverage Status
Change from base Build 3330: 0.0%
Covered Lines: 4100
Relevant Lines: 4923

💛 - Coveralls

json/to_json_test.mbt 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.

3 participants