moonbitlang / moon

The build system and package manager for MoonBit
https://moonbitlang.github.io/moon/
GNU Affero General Public License v3.0
188 stars 17 forks source link

support moon query #471

Closed Young-Flash closed 5 days ago

peter-jerry-ye-code-review[bot] commented 1 week ago

From the provided git diff output, it appears that the changes involve adding a new feature to the MoonBit CLI tool for querying module information and displaying build artifacts. Here are three potential issues or areas of concern that I've identified from the diff:

  1. Potential Bug in Artifact Display:

    • The new feature introduces a flag --show-artifacts to display build artifacts. The code for displaying artifacts uses a Vec to store artifact information and then serializes it to JSON. However, there's a comment mentioning that using HashMap or IndexMap is problematic due to ordering issues. This could potentially lead to issues if the order of packages is important for the user or if there are dependencies between packages that should be reflected in the output order.
    // can't use HashMap because the order of the packages is not guaranteed
    // can't use IndexMap because moonc cannot handled ordered map
    let mut artifacts = Vec::new();

    Suggestion: Ensure that the order in which artifacts are displayed is either documented as not guaranteed or implement a mechanism to guarantee a specific order (e.g., by sorting based on dependencies or package names).

  2. Potential Typo in Command Execution:

    • In the query.rs file, the moon_add_output and moon_build_output commands are executed. However, the error handling for these commands checks the status.success() and then directly prints the stdout. There's a potential typo or oversight where the stderr is printed only for moon_add_output but not for moon_build_output.
    if !moon_add_output.status.success() {
       eprintln!("{}", String::from_utf8_lossy(&moon_add_output.stdout));
       eprintln!("{}", String::from_utf8_lossy(&moon_add_output.stderr));
       anyhow::bail!("Failed to add module {}", mod_name);
    }
    println!("{}", String::from_utf8_lossy(&moon_build_output.stdout));
    if !moon_build_output.status.success() {
       anyhow::bail!("Failed to build module {}", mod_name);
    }

    Suggestion: Update the error handling for moon_build_output to also print the stderr, similar to moon_add_output.

  3. Potential Logic Issue in Topological Sorting:

    • The topological sorting function in module.rs constructs a graph and performs a topological sort to get the packages in dependency order. However, the error message for detecting a cyclic dependency mentions bail!("cyclic dependency detected: {:?}", cycle); which might not be informative enough for users to understand where the cycle occurs.
    Err(cycle) => {
       bail!("cyclic dependency detected: {:?}", cycle);
    }

    Suggestion: Improve the error message to provide more context about the cycle, such as the specific packages involved in the cycle. This could help users understand and resolve the issue more effectively.

These suggestions aim to improve the robustness and user experience of the new query feature added to the MoonBit CLI tool.