Skip to content

Comments

feat: [#612] install all facades#1203

Merged
hwbrzzl merged 5 commits intomasterfrom
bowen/install-all-facades
Sep 17, 2025
Merged

feat: [#612] install all facades#1203
hwbrzzl merged 5 commits intomasterfrom
bowen/install-all-facades

Conversation

@hwbrzzl
Copy link
Contributor

@hwbrzzl hwbrzzl commented Sep 17, 2025

📑 Description

Relate goravel/goravel#612

This pull request introduces several improvements and fixes to the package installation command, focusing on enhancing the facade installation workflow and its test coverage. The main changes include adding a new flag to install all facades, improving user interaction for facade selection, preventing duplicate installations within a single command execution, and updating related tests to cover these new scenarios.

Enhancements to facade installation workflow:

  • Added a new --all-facades (alias -a) flag to the PackageInstallCommand, allowing users to install all available facades at once without manual selection.
  • Improved the interactive prompt: when no arguments are provided and --all-facades is not set, users are now presented with a multi-select menu to choose one or more facades to install, with support for filtering and selecting all via keyboard shortcuts.
  • Added logic to prevent the same facade from being installed multiple times during a single command execution by tracking installed facades in the installedFacadesInTheCurrentCommand field. [1] [2] [3]
image

✅ Checks

  • Added test cases for my code

@almas-x
Copy link
Contributor

almas-x commented Sep 17, 2025

I have two questions regarding this PR:

  1. Since artisan is a base facade of the framework, it doesn’t support installation or uninstallation. Does this mean the setup directory is unnecessary for console package? Or should we allow users to manually run go run github.com/goravel/framework/console/setup install for installation?

  2. You mentioned the --all-facades option to install all facades. Would it be possible to provide a --facades option with a multi-selection list, so users can choose which facades to install as needed?

@hwbrzzl
Copy link
Contributor Author

hwbrzzl commented Sep 17, 2025

@almas-x Good question

  1. Yes, it can be removed.
  2. --facades may be unnecessary, the available facades can be chosen when no facade input.

@almas-x
Copy link
Contributor

almas-x commented Sep 17, 2025

Yes, it can be removed.
--facades may be unnecessary, the available facades can be chosen when no facade input.

If I remember correctly, when there is no facade or package input, it prompts to ask for the package or facade name, rather than selecting one. Based on the current “all-facades” logic, it either installs everything or requires entering the names one by one for installation.


dependencies := r.getDependenciesThatNeedInstall(binding)
if len(dependencies) > 0 {
if len(dependencies) > 0 && !ctx.OptionBool("all-facades") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Info is unnecessary when installing all facades.

@codecov
Copy link

codecov bot commented Sep 17, 2025

Codecov Report

❌ Patch coverage is 72.22222% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.96%. Comparing base (db0161d) to head (809b0c0).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
foundation/console/package_install_command.go 72.22% 9 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1203      +/-   ##
==========================================
+ Coverage   67.91%   67.96%   +0.05%     
==========================================
  Files         233      232       -1     
  Lines       14712    14721       +9     
==========================================
+ Hits         9991    10005      +14     
+ Misses       4363     4357       -6     
- Partials      358      359       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hwbrzzl hwbrzzl marked this pull request as ready for review September 17, 2025 12:12
@hwbrzzl hwbrzzl requested a review from a team as a code owner September 17, 2025 12:12
Copilot AI review requested due to automatic review settings September 17, 2025 12:12
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds the ability to install all facades at once through a new --all-facades flag and improves the facade selection experience by replacing the text prompt with a multi-select menu. It also prevents duplicate facade installations within a single command execution.

  • Added --all-facades flag to install all available facades without manual selection
  • Replaced text input prompt with interactive multi-select menu for facade selection
  • Added duplicate installation prevention by tracking installed facades per command

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
foundation/console/package_install_command.go Added --all-facades flag, multi-select UI, and duplicate prevention logic
foundation/console/package_install_command_test.go Updated tests to cover new flag, multi-select functionality, and duplicate prevention
database/setup/stubs.go Removed unused module parameter from Kernel method
database/setup/setup.go Updated call to simplified Kernel method
contracts/binding/facades.go Removed empty file
console/setup/setup.go Removed entire file

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

@almas-x almas-x left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@hwbrzzl hwbrzzl merged commit b7f7741 into master Sep 17, 2025
14 checks passed
@hwbrzzl hwbrzzl deleted the bowen/install-all-facades branch September 17, 2025 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants