Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1176 +/- ##
==========================================
- Coverage 68.86% 68.19% -0.68%
==========================================
Files 230 232 +2
Lines 14879 14993 +114
==========================================
- Hits 10246 10224 -22
- Misses 4235 4390 +155
+ Partials 398 379 -19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| func main() { | ||
| // TODO: optimize this in https://github.com/goravel/goravel/issues/447 | ||
| // config, err := supportfile.GetFrameworkContent("auth/setup/config/auth.go") |
There was a problem hiding this comment.
GetFrameworkContent is not a good choice, it's unstable.
| s.Equal("config1.go", s.app.publishGroups["public"]["config.go"]) | ||
| } | ||
|
|
||
| func (s *ApplicationTestSuite) TestMakeArtisan() { |
There was a problem hiding this comment.
Avoid cycle dependency.
| Dependencies: collect.Unique( | ||
| binding.Facades[binding.Auth].Dependencies, | ||
| binding.Facades[binding.Gate].Dependencies, | ||
| binding.Bindings[binding.Auth].Dependencies, |
There was a problem hiding this comment.
How about renaming binding.Bindings to something like binding.Builtin to avoid redundancy?
There was a problem hiding this comment.
em, I think Builtin is a bit meaningless.
There was a problem hiding this comment.
The term Builtin may seem meaningless on its own, but when combined as binding.Builtin, it clearly indicates that these are the framework’s built-in bindings. By the way binding.Bindings is also fine.
There was a problem hiding this comment.
Pull Request Overview
This pull request implements install/uninstall functionality for the Auth facade as part of issue #447. It refactors the packages setup system to support facade-specific installation with improved dependency management and better testing coverage.
Key changes include:
- Adds facade-specific installation/uninstallation with --facade flags
- Refactors service providers to use binding.Bindings instead of binding.Facades
- Introduces facade/binding conversion utilities and conditional modification logic
Reviewed Changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| auth/setup/setup.go | Implements Auth and Gate facade installation/uninstallation logic |
| auth/setup/stubs.go | Adds facade stub templates for Auth and Gate |
| packages/setup.go | Adds facade and force flag parsing for setup commands |
| packages/modify/modify.go | Adds conditional facade installation modifiers and options system |
| support/convert/facade.go | Utility functions for converting between facade names and bindings |
| contracts/binding/binding.go | Moves facade info from facades.go to bindings with Info struct |
| foundation/application.go | Adds Bindings() method and FacadesPath() support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
📑 Description
Relates to goravel/goravel#447
This pull request refactors the setup and uninstall logic for the authentication package and improves the handling and testing of facade dependencies. The main changes include moving configuration stubs to a dedicated struct, updating uninstall logic to prevent removal of facades that are depended on by others, and enhancing the test suite to cover these scenarios.
✅ Checks