Add Mago as task#1216
Conversation
veewee
left a comment
veewee
left a comment
There was a problem hiding this comment.
Thanks for the PR.
I think this task in current state is not in line with other tasks. I've added some remarks inline in the code. Feel free to stat a discussion.
| $resolver = new OptionsResolver(); | ||
| $resolver->setDefaults([ | ||
| 'formatter' => true, | ||
| 'formatter_options' => ['--staged'], |
There was a problem hiding this comment.
staged should only be a default during pre-commit I suppose?
| ]); | ||
|
|
||
| $resolver->addAllowedTypes('formatter', ['bool']); | ||
| $resolver->addAllowedTypes('formatter_options', ['array']); |
There was a problem hiding this comment.
In other tasks, we provide configuration keys that map the options of the CLI.
Doing it like this introduces a new way of configuring tasks, which I'dd like to avoid at this point.
| { | ||
| $config = $this->getConfig()->getOptions(); | ||
|
|
||
| if ($config['formatter'] === false |
There was a problem hiding this comment.
I was thinking: wouldn't it make more sense to introduce a task per type of command?
Something like following tasks:
mago_fmtmago_lintmago_analyzemago_guard
Every task will have it's own set of configurable options.
|
Hey @veewee, Thanks for taking the time to review the PR and share your feedback. To be fully transparent, after opening the PR I had already started reworking it to introduce 4 commands. I’ll update the PR this week with those changes; it should end up closer to how the SecurityChecker tasks are structured, which should resolve the three threads. If you have any additional feedback, feel free to share it so I can address everything at once. |
2 participants
This PR adds a Mago task
Mago is a recent tool that includes a formatter (like phpcs), a linter (like phpmd), and a static analyzer (like phpstan).
It is known for its performance and is starting to be increasingly adopted (for example, in Drupal).
Its integration into GrumPHP seems valuable.
New Task Checklist:
run()method readable?run()method using the configuration correctly?