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

fix: remove accidentally leaked apis #1393

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

Conversation

peter-jerry-ye
Copy link
Collaborator

No description provided.

Copy link

peter-jerry-ye-code-review bot commented Dec 31, 2024

‼️ 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. Potential Issue with moon check Command:

    • The moon check command has been updated to include the --target all flag. This change might affect the scope of the checks being performed. Ensure that this flag is intended and that it aligns with the project's testing strategy. If --target all is not necessary, it could lead to unnecessary overhead or unintended behavior.
  2. Visibility Changes in Type Definitions:

    • Several types (JSArray, JSValue, Int64WasmHelper) have been changed from public to private (priv). This could potentially break existing code that relies on these types being publicly accessible. Review the usage of these types across the codebase to ensure that this change does not introduce compilation errors or runtime issues.
  3. Addition of diff Command in moon info Step:

    • A new diff command has been added to compare the output of mooninfo for two different targets (js and wasm-gc). This addition is useful for ensuring consistency between different build targets. However, ensure that the paths (target/js/release/check/builtin/builtin.mi and target/wasm-gc/release/check/builtin/builtin.mi) are correct and that the files exist at these locations. If the paths are incorrect, this command will fail.

These changes seem to be focused on improving the robustness and maintainability of the code, but they should be carefully reviewed to avoid introducing new issues.

@coveralls
Copy link
Collaborator

coveralls commented Dec 31, 2024

Pull Request Test Coverage Report for Build 4564

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 83.295%

Totals Coverage Status
Change from base Build 4551: 0.0%
Covered Lines: 4727
Relevant Lines: 5675

💛 - Coveralls

@@ -177,7 +177,7 @@ pub fn Double::reinterpret_as_u64(self : Double) -> UInt64 = "%f64_to_i64_reinte
///|
pub fn Double::reinterpret_as_uint64(self : Double) -> UInt64 = "%f64_to_i64_reinterpret"

///|
///| @alert deprecated "Use `UInt64::to_double` instead"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UInt64 is not safe to represent in Double. I think UInt64::to_double is not a good name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is as good / bad as (int)1.0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not do two things in a single PR, can you split your changes (as tilte) into a single PR?

@peter-jerry-ye peter-jerry-ye changed the title ci: show inconsistent api fix: remove accidentally leaked apis Dec 31, 2024
@hackwaly hackwaly requested a review from bobzhang December 31, 2024 05:57
@peter-jerry-ye peter-jerry-ye marked this pull request as draft January 10, 2025 05: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.

4 participants