Skip to content
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

Upgrade #62

Merged
merged 10 commits into from
Feb 16, 2024
Merged

Upgrade #62

merged 10 commits into from
Feb 16, 2024

Conversation

kbond
Copy link
Member

@kbond kbond commented Feb 15, 2024

  • Require PHP 8.1+
  • Require Symfony 6.4+
  • deprecate injecting options/arguments w/o attribute
  • change default name of options to kebab-case #41
  • Add abstract class InvokableCommand extends Command
  • Deprecate Invokable trait (extend InvokableCommand instead)?
  • Deprecate ConfigureWithAttributes trait (would be moved to InvokableCommand)?
  • Have InvokableCommand use ConfigureWithAttributes
  • Update docs
  • Auto-complete / suggested values #38

Closes #38

@kbond
Copy link
Member Author

kbond commented Feb 15, 2024

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

@tacman
Copy link
Contributor

tacman commented Feb 15, 2024

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
Copy link
Member Author

kbond commented Feb 15, 2024

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
Copy link
Contributor

tacman commented Feb 15, 2024

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
Copy link
Member Author

kbond commented Feb 15, 2024

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 kbond marked this pull request as ready for review February 15, 2024 20:46
@kbond
Copy link
Member Author

kbond commented Feb 15, 2024

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.

@kbond kbond merged commit 3a4017d into zenstruck:1.x Feb 16, 2024
13 checks passed
@kbond kbond deleted the upgrade branch February 16, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Auto-complete / suggested values
2 participants