staabm / phpstan-todo-by

Todo comments with expiration
https://staabm.github.io/2023/12/17/phpstan-todo-by-published.html
MIT License
173 stars 4 forks source link

Add support for YouTrack in TodoByTicketRule #51

Closed DannyvdSluijs closed 8 months ago

DannyvdSluijs commented 8 months ago

This adds support for YouTrack in the the TodoByTicketRule.

There is still some unresolved issue at this point in time for which some input could help regarding the approach. With this PR there are now two implementations of the staabm\PHPStanTodoBy\utils\TicketStatusFetcher causing the Nette autowiring to throw an exception:

Multiple services of type staabm\PHPStanTodoBy\utils\TicketStatusFetcher found: 0397, 0398

@staabm I guess I could add an enabled flag on each of the ticket fetchers config and add entries to the conditionalTags section of the extension.neon file. But this could lead two multiple issue trackers being enabled, which isn't supported by the rule. Also the conditionalTags seems undocumented which results in me maybe not seeing all the options here. Maybe this touches https://github.com/staabm/phpstan-todo-by/issues/48 as well.

I'm willing to continue the PR in order to get it to the finish line but could use some of you input in order to go in the right direction.

staabm commented 8 months ago

thanks for the PR.

regarding youtrack in general: does youtrack use the same issue-key-format as in jira? meaning a project identifier followed by a number (will the jira regex work for youtrack as is?)

I guess I could add an enabled flag on each of the ticket fetchers config and add entries to the conditionalTags section of the extension.neon file.

I think we should instead utilize the enabled flag and turn it into a string parameter.

staabm commented 8 months ago

There is still some unresolved issue at this point in time for which some input could help regarding the approach. With this PR there are now two implementations of the staabm\PHPStanTodoBy\utils\TicketStatusFetcher causing the Nette autowiring to throw an exception:

I think we could typehint the concrete implementations to make the dependencies unambigous

EmilMassey commented 8 months ago

Hi, nice to see upcoming support for another issue tracker.

Recently I started working on introducing GitHub support (which has completely different ticket keys: #123) and this requires some refactoring to be able to support multiple issue trackers. See the draft: #62. To be able to choose a fetcher dynamically, we can use factory which has DI container injected. This way we can read parameters to decide which service to use.

Adding your fetcher would be as easy as adding those lines to the factory:

if ('youtrack' === $tracker) {
    $fetcher = $this->container->getByType(YouTrackTicketStatusFetcher::class);

    return new TicketRuleConfiguration(
        $fetcher::getKeyPattern(),
        $resolvedStatuses,
        $keyPrefixes,
        $fetcher,
    );
}

What do you think?

staabm commented 8 months ago

I like the direction of https://github.com/staabm/phpstan-todo-by/pull/62 - we should wait with this PR here until #62 is merged

DannyvdSluijs commented 8 months ago

I see now #62 is merged so I can rebase/redo this work with minimal effort. Great work by @EmilMassey this was the part I was missing.

I’ll try and see if I can update this PR in the upcoming week.

staabm commented 8 months ago

could we have a similar e2e test for youtrack, as we have added recently for jira?

https://github.com/staabm/phpstan-todo-by/pull/71

staabm commented 8 months ago

@EmilMassey any input from your side?

staabm commented 8 months ago

thank you guys