8000 feat: add sub-package support by Young-Flash · Pull Request #816 · moonbitlang/moon · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: add sub-package support #816

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 21, 2025
Merged

feat: add sub-package support #816

merged 2 commits into from
May 21, 2025

Conversation

Young-Flash
Copy link
Collaborator

@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
peter-jerry-ye-code-review bot commented May 20, 2025 •
edited
Loading
Missed package name validation in sub-package handling

Category
Maintainability
Code Snippet
pub struct SubPackageInMoonPkg {
pub files: Vec,
pub import: Vec,
}
Recommendation
Add validation to ensure sub-package files exist and have unique names within the package scope
Reasoning
The sub-package implementation accepts a list of files without validating their existence or uniqueness. This could lead to runtime errors or confusing behavior if files are missing or duplicated.

Potential path manipulation issue with sub-package postfix

Category
Correctness
Code Snippet
*last = format!("{}{}", last, SUB_PKG_POSTFIX);
Recommendation
Add sanitization for package names to ensure they can safely accept the SUB_PKG_POSTFIX without creating invalid paths
Reasoning
Directly appending SUB_PKG_POSTFIX to package names could create invalid paths if the package name contains special characters or is near path length limits

Test case does not verify error handling

Category
Maintainability
Code Snippet
fn test_sub_package() {
let dir = TestDir::new("test_sub_package.in");
// ...
}
Recommendation
Add negative test cases to verify proper error handling for invalid sub-package configurations
Reasoning
The test only covers the happy path. Missing error cases like invalid file paths, duplicate sub-packages, or circular dependencies could lead to uncaught bugs

@Young-Flash Young-Flash force-pushed the sub_package branch 2 times, most recently from a513230 to a45ddae Compare May 20, 2025 06:21
Young-Flash
Young-Flash commented May 21, 2025
View reviewed changes
crates/moon/tests/test_cases/mod.rs Outdated
expect![[r#"
moonc build-package ./dep/hello.mbt -o ./target/wasm-gc/debug/test/dep/dep.core -pkg moon_new/dep -std-path $MOON_HOME/lib/core/target/wasm-gc/release/bundle -pkg-sources moon_new/dep:./dep -target wasm-gc -g -O0
moon generate-test-driver --source-dir . --target-dir ./target --package moon_new/test --sort-input --target wasm-gc --driver-kind internal --mode test
moonc build-package ./sub_pkg/111.mbt ./sub_pkg/dir/222.mbt -o ./target/wasm-gc/debug/test/sub_pkg/sub_pkg_sub.core -pkg moon_new/sub_pkg_sub -std-path $MOON_HOME/lib/core/target/wasm-gc/release/bundle -i ./target/wasm-gc/debug/test/dep/dep.mi:dep -pkg-sources moon_new/sub_pkg_sub:./sub_pkg -target wasm-gc -g -O0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

-pkg moon_new/sub_pkg_sub --> -pkg moon_new/sub_pkg

@Young-Flash Young-Flash enabled auto-merge May 21, 2025 09:08
@Young-Flash Young-Flash merged commit c8aff08 into main May 21, 2025
13 checks passed
@Young-Flash Young-Flash deleted the sub_package branch May 21, 2025 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@Young-Flash
0