zenstruck / console-extra

A modular set of features to reduce configuration boilerplate for your Symfony commands.
MIT License
78 stars 3 forks source link

Upgrade #62

Closed kbond closed 8 months ago

kbond commented 8 months ago

Closes #38

kbond commented 8 months ago

Hey @tacman. I want to cleanup this package and create a 2.0 release. What do you think of my proposed todos above?

tacman commented 8 months ago

Well, if you were going for a 2.0 release, lemme run one thing by you.

The Argument Attribute takes the following arguments

 public function __construct(
        public ?string $name = null,
        private ?int $mode = null,
        private string $description = '',
        private string|bool|int|float|array|null $default = null,
    ) {
    }

but 3 of those 4 arguments are inferred from the property that follows it. Description is such a long word, and phpcs is constantly whining about my lines being too long, but I like having everything on one line, eg.

    public function __invoke(
        IO                                                                                    $io,
        #[Argument(description: 'search query for packages, e.g.type=symfony-bundle')] string $q = 'type=symfony-bundle',
        //        #[Option(description: 'scrape the package details')] bool                             $details = false,
        #[Option(description: 'load the bundle names and vendors')] bool                      $setup = false,
        #[Option(description: 'fetch the latest version json')] bool                          $fetch = false,
        #[Option(description: 'process the json in the database')] bool                       $process = false,
        #[Option(description: 'page size')] int                                               $pageSize = 100

It actually shorter to pass in null, null, "description" than to call it with the named argument.

So even though it's counter-intuitive, I'd like $description to be first.

On a related note, can you even pass in $mode?

Traits v. extending seems fine to me, but I'm no expert there.

I was running into an issue trying to run composer from runCommand. Unlikely to be related to 1.0 or 2.0, so I'll open it in a separate issue, but I think I was hoping for more debugging.

Thanks for releasing this! Many of my bundles depend on this, I'm always hesitant to add another dependency, but I really like the clean setup.

kbond commented 8 months ago

So even though it's counter-intuitive, I'd like $description to be first.

I don't think there's a way to create a deprecation path for this. Here I'm thinking the solution would be using multiple lines:

#[Argument(description: 'search query for packages, e.g.type=symfony-bundle')]
string $q = 'type=symfony-bundle',

// or even
#[Argument(
    description: 'search query for packages, e.g.type=symfony-bundle',
)]
string $q = 'type=symfony-bundle',

On a related note, can you even pass in $mode?

Yep, it's important when using these attributes at the class level.

Traits v. extending seems fine to me, but I'm no expert there.

Ok, my concern with removing the trait is: if you have a command that extends another custom base command, you'd no longer be able to make it invokable. To me, if feels like a rare or non-existent scenario but wanted to get your thoughts.

tacman commented 8 months ago

Yes, I know how to avoid the long line problem, but wanted to skip the named argument. I can write my own Argument attribute, though, and reorder it.

I would like to extend a custom command, mostly to not have to repeat certain arguments and options. But I don't think what you're proposing precluded that.

kbond commented 8 months ago

Yes, I know how to avoid the long line problem, but wanted to skip the named argument. I can write my own Argument attribute, though, and reorder it.

Ok, we'll have to allow for this (make the attributes non-final)

kbond commented 8 months ago

Ok, I think this is good to go. I opted not to deprecate the Invokable/ConfigureWithAttributes traits but updated the docs to remove any reference to them.

So now:

  1. Not using Symfony? Extend InvokableCommand
  2. Using Symfony? Extend InvokableServiceCommand

I trigger a deprecation advising user's remove these traits if they're not needed (ie, your command extends InvokableCommand and you use one of these traits).

Ok, we'll have to allow for this (make the attributes non-final)

The attributes are now extendable but I opted not to document this to keep things simple.