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

Expire comment by config value #42

Closed glaubinix closed 8 months ago

glaubinix commented 8 months ago

I am not sure if using config values is the best way to achieve this, but I have the following uses cases where something like this would be very helpful:

I was thinking about a section in the PHPStan config like:

parameters:
    todo_by:
        values: # or something more helpful
            minimumApiClientVersion: 1.2

and then near the code path

// TODO: minimumApiClientVersion:2 This can be removed once all API consumers use client version 2.0 or higher
staabm commented 8 months ago

should be easy to implement.

I am also thinking about a good name because values is a very generic and multi purposed term. maybe versionToggles could work?

the idea is to describe constraints, similar its done for packages in composer.json , right?


we are also working on a issue-tracker integration. that way you could manage your current planning state in the ticket and as soon as you close the ticket (or move it into one of the configured 'closed-states'), comments would invalidate.

the mentioned PR integrates jira, but for sure we could do the same for github issues or similar stuff.

would this work for you, too?

glaubinix commented 8 months ago

Yes, constraints similar to packages in composer.json would be perfect! versionToggles already sounds like a better name than the generic values.

I was thinking of a config file because the step to increase the constraints could be automated by running some command that then automatically creates a PR to adjust the config file. The PR would fail and force us to clean up the code paths.

Issue tracker integration could probably work too. The main downsides I see with that approach is that it requires API credentials to run and it does API requests during the PHPStan run which can fail and will slow down the PHPStan run. It also requires you to remember that an issue actually exists which might be a problem, considering that some of them take >1 year to resolve.

Seldaek commented 8 months ago

I guess I'd call it virtualPackages: or in Composer term we'd say provide: to declare additional package names with their versions :) But just my 2c, if you think it reads weird from your perspective you're probably right. I'm biased.

staabm commented 8 months ago

thx for the input. this makes me think whether we should have the composer name scheme applied for the packages and therefor do it like:

parameters:
    todo_by:
      virtualPackages:
        - 'apiClient/minimumVersion': '1.2'   

meaning we would need a x/y notated package.

we could then just re-use the existing rule

glaubinix commented 8 months ago

Sounds like a great idea! Less code to maintain should make it easier for everyone.

github-actions[bot] commented 7 months ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.