-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add explicit support for #[Command], #[Hook], and more PHP8 Attributes #223
Conversation
Codecov Report
@@ Coverage Diff @@
## main #223 +/- ##
============================================
+ Coverage 89.74% 89.76% +0.02%
+ Complexity 741 731 -10
============================================
Files 45 45
Lines 1921 1896 -25
============================================
- Hits 1724 1702 -22
+ Misses 197 194 -3
Continue to review full report at Codecov.
|
…ram to just take a description. The default comes from method params not attribute/annoation.
…clare their Attribute class.
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.
I'm happy to proceed with this PR, but I have a feeling we will want to end up with multiple annotations on a method, not a single CommandLineAttributes with many properties. For example, see Doctrine. |
ah, interesting. |
At first I thought that having a lot of distinct attributes wouldn't be good, but after looking at the Doctrine example, maybe it's better that way. It has the added advantage of not requiring the use of multi-line attributes, although in truth I don't think we'd ever want to encourage people to write commands that used BOTH docblocks AND attributes at the same time, so that's maybe not a huge factor. Maybe something like this:
Or, equivalently:
Can we have multiple instances of the same attribute, e.g. if we wanted multiple |
Oh, and getting rid of the |
I fixed up the handling of options in the attribute parser, so that they may now be passed as ordinary method parameters. See #230 |
I realize that my |
I did a non-definitive test. I think that the example below would probably work:
|
AnnotatedCommandFactoryTest is more thorough than AttributesCommandFactoryTest. I think a followup could refactor so we get same assertions in each. |
With this latest push, Attribute classes have a |
…to output-formatters repo.
src/Attributes/Generic.php
Outdated
|
||
public static function handle(\ReflectionAttribute $attribute, CommandInfo $commandInfo) | ||
{ | ||
$commandInfo->addAnnotation(static::NAME, null); |
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.
This is an example attribute, not a general-purpose attribute, I suppose?
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.
Its a base class. Its used by https://github.com/drush-ops/drush/pull/4821/files#diff-2a2e5ff65cd12399a0f627fc3881b2445974d8e0717fcb4f0c894c09fe4681de, for example. I renamed to NoArgumentsBase and made it abstract. This class is not used by AC so it could easily move to Drush if preferred.
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.
Looks pretty neat, just a couple style comments.
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.
OK to merge after removing or explaining NoArgumentsBase
Also adds php8 constructor properties to help IDE complete attribute names.
Needed for drush-ops/drush#4798
Disposition
This pull request: