staabm / phpstan-todo-by

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

support full github issue urls #91

Closed staabm closed 3 months ago

staabm commented 6 months ago

I think we should support comments with a full github url:

// TODO: https://github.com/staabm/phpstan-todo-by/issues/91 we need todo something when this issue is resolved

I think these are helpful, because the urls represent a ticket uniquely.


in contrast, comments like

// TODO: some-organization/some-repo#123 change me if this GitHub pull request is closed

have the problem, that you need to know how phpstan-todo-by is configured, to reason what the correct url for this relative identifier is.


I think we should even detect/verify these full url todos when the issue tracker is not configured to be github. my guess is that github.com urls would be the lions share of urls used in comments.

the only other public issue tracker which might be similar widely used is gitlab.com.

turned differently: IMO when a todo comment is followed by a full url, we should work against a whitelist of default providers (no matter what is configured to be "the default issue tracker" in the config file.)

staabm commented 6 months ago

@EmilMassey do you have an opinion on this feature request?

EmilMassey commented 6 months ago

I think it's a very good idea. I like the suggestion to always detect issues when referenced with full URL, regardless of the configuration. When it's implemented, developers in some cases wouldn't even have to configure trackers for this rule to work, am I right?

If fetching ticket status by URL is always enabled, then I think the PR implementing this should deprecate the config parameter tracker and replace it with default_tracker or something like to better reflect its purpose.

At the moment you can only use one provider, but if I understand your idea correctly, it will no longer be the case, because some (or all?) providers will always be enabled. Should we allow for configuring multiple trackers? I wonder what are implications of this.

I can't imagine codebase that uses both: youtrack and jira, but if there was one, we might not be able to tell where the key (e.g. APP-1234) should be looked up. Maybe we'd need to call both providers then which is problematic due to performance. But I think it's not a rare scenario and I know projects where the project primarily uses private Jira board and references todos in a relative way, but also has some comments regarding dependencies and for this uses the full GitHub / gitlab / whatever URLs. To access private Jira board, credentials must be configured. And if you have more than 60 GitHub todos, then you will also need to configure credentials for GitHub, because unauthenticated calls are limited to 60 per hour if I remember correctly.

In conclusion, I think it will be very useful, but it needs to be well thought out and some decisions need to be made at this stage so as not to increase the complexity excessively.

staabm commented 6 months ago

I think it's a very good idea. I like the suggestion to always detect issues when referenced with full URL, regardless of the configuration. When it's implemented, developers in some cases wouldn't even have to configure trackers for this rule to work, am I right?

exactly, it should work out of the box

If fetching ticket status by URL is always enabled, then I think the PR implementing this should deprecate the config parameter tracker and replace it with default_tracker or something like to better reflect its purpose.

agree

At the moment you can only use one provider, but if I understand your idea correctly, it will no longer be the case, because some (or all?) providers will always be enabled. Should we allow for configuring multiple trackers? I wonder what are implications of this.

I am not sure we need it - and I would leave it like it is for now. Projects with several tracker can use absolute urls for now. Lets wait and see whether there is real demand for a key-only based multi tracker setup

github-actions[bot] commented 2 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.