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

Refactor method 'add_cli' to reduce duplicated code #307

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

lgxbslgx
Copy link
Contributor

This patch reduces the duplicated code in add_cli, changes the parameters of the method add_latest and removes the parameter _registry_config of the method add. Thanks for your review.

Related Issues

  • Related issues: #____

Type of Pull Request

  • Bug fix
  • New feature
    • I have already discussed this feature with the maintainers.
  • Refactor
  • Performance optimization
  • Documentation
  • Other (please describe):

Does this PR change existing behavior?

  • Yes (please describe the changes below)
  • No

Does this PR introduce new dependencies?

  • No
  • Yes (please check binary size changes)

Checklist:

  • Tests added/updated for bug fixes or new features
  • Compatible with Windows/Linux/macOS

Copy link

peter-jerry-ye-code-review bot commented Sep 14, 2024

Identified Issues in deps.rs

  1. Redundant Code for Splitting and Validating Package Path:

    • The initial splitting of the package_path into parts and then again into author_pkg is redundant. The same validation can be done in a single pass.
    • Suggestion: Simplify the validation logic to avoid unnecessary splitting and validation steps.
  2. Inconsistent Handling of Package Path:

    • The handling of the package path is split into two cases based on the presence of a version, leading to similar but slightly different logic paths.
    • Suggestion: Consolidate the logic for handling the package path to reduce redundancy and potential inconsistencies.
  3. Error Message Consistency:

    • The error message for invalid package path formats is slightly different in the two branches (if parts.len() == 2 and else).
    • Suggestion: Standardize the error message format to improve readability and user experience.

Identified Issues in add.rs

  1. Unused Parameter:

    • The parameter _registry_config: &RegistryConfig is defined but never used in the add function.
    • Suggestion: Remove the unused parameter to simplify the function signature and avoid confusion.
  2. Redundant String Formatting:

    • The comparison if format!("{username}/{pkgname}") == MOONBITLANG_CORE is redundant. The ModuleName struct already contains this information.
    • Suggestion: Compare directly with pkg_name.to_string() to improve readability and performance.
  3. Error Message Clarity:

    • The error message for failing to find the latest version of a package is somewhat verbose and repetitive.
    • Suggestion: Simplify the error message to provide clearer feedback to the user.

By addressing these issues, the code can be made more concise, readable, and maintainable.

Copy link
Contributor

@lijunchen lijunchen left a comment

Choose a reason for hiding this comment

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

thanks

@lijunchen lijunchen enabled auto-merge (squash) September 18, 2024 08:20
@lijunchen lijunchen merged commit 6bc960e into moonbitlang:main Sep 18, 2024
9 checks passed
@lgxbslgx lgxbslgx deleted the REFACTOR_ADD branch September 18, 2024 09:48
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.

2 participants