feat: add the relationship of facades and bindings#1233
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new centralized facades package that standardizes the mapping between facade names and their binding keys, replacing hardcoded string literals and string manipulation logic throughout the codebase.
- Adds a new
contracts/facades/facades.gofile with facade constants and aFacadeToBindingmap - Updates container methods to use the new facade-to-binding mapping instead of hardcoded strings
- Refactors facade/binding conversion functions to use the centralized mapping
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
contracts/facades/facades.go |
New file defining facade constants and mapping to binding keys |
foundation/container.go |
Updates all Make* methods to use the new facade-to-binding mapping |
support/convert/facade.go |
Replaces string manipulation logic with map-based lookups |
auth/setup/setup.go |
Updates setup logic to use facade constants instead of string literals |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: cddc195 | Previous: 2d07c62 | Ratio |
|---|---|---|---|
Benchmark_EncryptString |
5093 ns/op 2152 B/op 14 allocs/op |
1610 ns/op 2152 B/op 14 allocs/op |
3.16 |
Benchmark_EncryptString - ns/op |
5093 ns/op |
1610 ns/op |
3.16 |
This comment was automatically generated by workflow using github-action-benchmark.
|
|
||
| import "github.com/goravel/framework/contracts/binding" | ||
|
|
||
| const ( |
There was a problem hiding this comment.
Is there any chance that the facades and bindings will differ? The mapping seems to be the same? We can assume binding as facade?
There was a problem hiding this comment.
Yes, currently, facades.Auth -> binding.Auth by default, but it can be facades.Auth -> binding.AnotherAuth in the future.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1233 +/- ##
==========================================
- Coverage 66.35% 66.33% -0.03%
==========================================
Files 245 245
Lines 16914 16922 +8
==========================================
+ Hits 11224 11225 +1
- Misses 5308 5315 +7
Partials 382 382 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📑 Description
This pull request introduces a new centralized
facadespackage to standardize the mapping between facade names and their binding keys. It updates the container logic and related utility functions to use this new mapping, leading to cleaner and more maintainable code. The changes also refactor the setup and conversion logic to utilize the new constants and mappings.facades.Authcan be used instead of using anAuthstring directly.✅ Checks