Skip to content

feat: [#667 #719] Add setup and relationship for all modules#1100

Merged
hwbrzzl merged 9 commits intomasterfrom
bowen/#667-719
Jun 30, 2025
Merged

feat: [#667 #719] Add setup and relationship for all modules#1100
hwbrzzl merged 9 commits intomasterfrom
bowen/#667-719

Conversation

@hwbrzzl
Copy link
Contributor

@hwbrzzl hwbrzzl commented Jun 29, 2025

📑 Description

Closes goravel/goravel#667
Related goravel/goravel#719

  1. Skip the package:install facade feature, will implement it in v1.17
  2. Add configuration files for all modules, will optimize other parts in v1.17

✅ Checks

  • Added test cases for my code

@almas-x
Copy link
Contributor

almas-x commented Jun 29, 2025

The current config/config.go files in various modules are overwritten into the project’s config directory after running artisan install xxx. This makes the original config.go files in the modules unnecessary. However, I’m unsure whether these config.go files are included during the go build process. Should we consider using build tags to exclude them from the normal go build process?

@hwbrzzl
Copy link
Contributor Author

hwbrzzl commented Jun 30, 2025

@almas-x Yes, I'll create another PR to remove the configuration files in goravel/goravel. These new config.go files should not be built because their packages are not quoted. Previously, I wanted to publish #1099 into v1.16, but I found there are various works that need to be done, it's hard to complete it in v1.16. Hence I moved it to v1.17 in order to get more time to optimize it.

@hwbrzzl
Copy link
Contributor Author

hwbrzzl commented Jun 30, 2025

But for this PR, it can be merged when it's ready. No impact on v1.16.

@codecov
Copy link

codecov bot commented Jun 30, 2025

Codecov Report

Attention: Patch coverage is 3.26797% with 888 lines in your changes missing coverage. Please review.

Project coverage is 66.79%. Comparing base (7b332a1) to head (a431fec).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
session/setup/config/session.go 0.00% 83 Missing ⚠️
http/setup/config/http.go 0.00% 39 Missing ⚠️
mail/setup/config/mail.go 0.00% 38 Missing ⚠️
queue/setup/config/queue.go 0.00% 37 Missing ⚠️
log/setup/config/logging.go 0.00% 35 Missing ⚠️
http/setup/config/jwt.go 0.00% 34 Missing ⚠️
hash/setup/config/hashing.go 0.00% 33 Missing ⚠️
cache/setup/config/cache.go 0.00% 30 Missing ⚠️
filesystem/setup/config/filesystems.go 0.00% 29 Missing ⚠️
http/setup/setup.go 0.00% 28 Missing ⚠️
... and 38 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1100      +/-   ##
==========================================
- Coverage   71.15%   66.79%   -4.37%     
==========================================
  Files         185      212      +27     
  Lines       13213    14011     +798     
==========================================
- Hits         9402     9358      -44     
- Misses       3441     4283     +842     
  Partials      370      370              

☔ 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 June 30, 2025 07:28
Copilot AI review requested due to automatic review settings June 30, 2025 07:28
@hwbrzzl hwbrzzl requested a review from a team as a code owner June 30, 2025 07:28
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 PR adds scaffolding commands and introduces a Relationship() API for all framework modules, centralizes facade mappings, and marks facade install/uninstall support as a TODO for v1.17.

  • Added setup/main.go scripts for each module to install/uninstall providers and config files.
  • Updated all service_provider.go files to implement Relationship() instead of separate Bindings(), Dependencies(), and ProvideFor().
  • Commented out facade install/uninstall in console commands and tests, deferring full implementation to v1.17.
  • Introduced contracts/binding/binding.go with centralized constants and FacadeToPath mapping and removed legacy facade files.

Reviewed Changes

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

Show a summary per file
File Description
*/setup/setup.go (all modules) New CLI scaffolding for install/uninstall of each module
*/service_provider.go (all modules) Added Relationship() method to describe bindings and deps
foundation/console/package_install_command.go Commented out facade install logic, marked as TODO
foundation/console/package_uninstall_command.go Commented out facade uninstall logic, marked as TODO
foundation/console/*_command_test.go Commented out facade tests, reduced test coverage
contracts/binding/binding.go Added central Binding constants, FacadeToPath, and struct
facades/*.go Removed individual facade files in favor of consolidated one
Comments suppressed due to low confidence (4)

foundation/console/package_install_command_test.go:122

  • [nitpick] Facade installation tests are entirely commented out. To maintain test coverage, add or update tests for the new facade installation paths once implemented.
		// TODO: Implement this in v1.17 https://github.com/goravel/goravel/issues/719

contracts/binding/binding.go:32

  • [nitpick] The FacadeToPath map uses title-case keys (e.g., "Auth"). CLI input and code references may use lowercase—consider normalizing keys or documenting expected casing to avoid mismatches.
var FacadeToPath = map[string]string{

foundation/console/package_uninstall_command.go:72

  • Facade uninstall logic is commented out, so non-package identifiers will always be handled as package uninstalls. Restore or replace this block to correctly route between package and facade uninstallation.
	// if isPackage(pkg) {

foundation/console/package_install_command.go:65

  • Facade install logic is commented out, causing all installs to use the package path flow. Implement the appropriate branch so facade identifiers are installed via their setup scripts.
	// if isPackage(pkg) {

almas-x
almas-x previously approved these changes Jun 30, 2025
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 bd4544f into master Jun 30, 2025
13 of 15 checks passed
@hwbrzzl hwbrzzl deleted the bowen/#667-719 branch June 30, 2025 12:52
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.

Registering service providers don't dependent order

3 participants