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(moon): (wip) support external subcommands #454

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rami3l
Copy link
Contributor

@rami3l rami3l commented Nov 7, 2024

This PR brings moon closer to git and cargo's behavior WRT external subcommands, that is, moon <subcmd> will fall back to calling moon-{name} if such a binary is found in PATH.

This encourages exploration of tooling ideas without relying on moon's codebase itself while keeping a unified user experience. Famous examples include git-branchless, cargo-binstall, cargo-chef and cargo-zigbuild.

See https://doc.rust-lang.org/cargo/reference/external-tools.html#custom-subcommands for more discussion on the rationale of this feature.

Copy link

From the provided git diff output, here are three observations and suggestions related to potential issues or improvements:

  1. Missing Documentation for New CLI Command:

    • The addition of the external module and its integration into the CLI (crates/moon/src/cli.rs) suggests the introduction of a new feature for handling external subcommands. However, there are no doc comments (///) or #[doc] attributes provided for the new external module or the run_external function. This could lead to confusion for other developers or users who need to understand how to use this new feature.
    • Suggestion: Add comprehensive doc comments to the external module and the run_external function. Describe what the feature does, how it should be used, and any specific considerations or requirements. Use /// or #[doc] attributes to provide this documentation directly in the code.
  2. Error Handling in run_external Function:

    • The run_external function uses anyhow::bail and anyhow::Context for error handling, which is good practice for ensuring that errors are informative and context-rich. However, there's a potential issue with the error message formatting in the which_global call. The error message uses anyhow::format_err!, which might not be necessary given that context is used to add context to errors.
    • Suggestion: Consider simplifying the error handling by relying more on context to add context to errors, rather than using format_err!. This can make the code cleaner and more consistent with Rust's error handling best practices. For example, replace anyhow::format_err! with context to add context to the error.
  3. Platform-Specific Code in exec Function:

    • The exec function is implemented differently for Unix and Windows platforms. While this is necessary for handling platform-specific nuances, the implementation for Windows includes an unsafe block and direct calls to Windows API functions (windows_sys::Win32::System::Console::SetConsoleCtrlHandler). This could introduce potential vulnerabilities or bugs if not handled carefully.
    • Suggestion: Ensure that the unsafe block is fully justified and that all potential risks are mitigated. Consider adding comments within the unsafe block to explain why it's necessary and what precautions have been taken. Additionally, review the Windows API documentation to ensure that the usage of SetConsoleCtrlHandler is correct and that there are no alternative, safer ways to achieve the same goal.

These suggestions aim to improve code clarity, error handling, and platform-specific code robustness, ensuring that the new feature is well-documented and less prone to bugs or misunderstandings.

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.

1 participant