-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Use constants for arbitrary annotation values #2675
Conversation
ae3fdd3
to
32a8715
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2675 +/- ##
==========================================
- Coverage 79.22% 79.20% -0.02%
==========================================
Files 161 161
Lines 8412 8412
==========================================
- Hits 6664 6663 -1
- Misses 1748 1749 +1 ☔ View full report in Codecov by Sentry. |
a86e342
to
fc4c9ec
Compare
fc4c9ec
to
7facd0b
Compare
namespace Gedmo\Mapping\Annotation; | ||
|
||
/** | ||
* @internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you expect projects to use those constants, I don't think it should be internal
/** | ||
* @internal | ||
*/ | ||
interface TrackingAwareAnnotationInterface extends Annotation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of an interface extending Annotation, I would suggest putting those constants on an enum-like class (being final and forbidding instantiation thanks to a private constructor).
If the PHP requirements were allowing it, those events would be a good candidate for an actual enum, as it is a closed list.
public const STYLE_LOWER = 'lower'; | ||
public const STYLE_UPPER = 'upper'; | ||
public const STYLE_CAMEL = 'camel'; | ||
public const STYLE_ORIGINAL = 'default'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those styles being a closed list also look like a good candidate for an enum class
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
To Do: