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: smart constructor for Json #1418

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

Conversation

peter-jerry-ye
Copy link
Collaborator

No description provided.

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 and potential issues from the provided git diff output:

  1. Inconsistent Return Statements:

    • In the Json::boolean function, the return statements are implicit (i.e., True and False are used without the return keyword). However, in other functions like Json::null, Json::number, Json::string, Json::array, and Json::object, the return keyword is explicitly used. This inconsistency in style could lead to confusion or readability issues. It would be better to standardize the use of return across all functions.
  2. Potential Typo in Example Code:

    • In the Json::array function's example, the array values is initialized with [1.0, "hello"]. However, 1.0 is a Double, and "hello" is a String. This array should contain elements of type Json, but the example does not explicitly wrap these values in Json types (e.g., Json::number(1.0) and Json::string("hello")). This could lead to a type mismatch or confusion for users.
  3. Missing Documentation for Json::default:

    • The Json::default function is declared in the impl Json block, but there is no corresponding implementation or documentation in the json.mbt file. This could be an oversight, as the function is part of the public API but lacks clarity on its purpose and usage. Adding documentation or an implementation for Json::default would improve the usability of the code.

These issues should be addressed to ensure consistency, correctness, and clarity in the codebase.

@coveralls
Copy link
Collaborator

coveralls commented Jan 3, 2025

Pull Request Test Coverage Report for Build 4685

Details

  • 0 of 8 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 83.101%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/json.mbt 0 8 0.0%
Totals Coverage Status
Change from base Build 4683: -0.1%
Covered Lines: 4765
Relevant Lines: 5734

💛 - Coveralls

@peter-jerry-ye peter-jerry-ye force-pushed the zihang/json-constructors branch 4 times, most recently from fa36822 to 7c36da3 Compare January 8, 2025 09:27
@peter-jerry-ye peter-jerry-ye marked this pull request as ready for review January 8, 2025 09:41
@peter-jerry-ye peter-jerry-ye force-pushed the zihang/json-constructors branch from c1f232b to c2e91a7 Compare January 10, 2025 05:47
@bobzhang
Copy link
Contributor

I would like to merge this but it is a bit dangerous currently since boolean, array would pollute the builtin namespace, maybe we can wait until the new method mechanism landed cc @Guest0x0

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