Add TodoByTicketRule (issue tracker support) #26
Conversation
| 'Authorization' => "Basic {$this->credentials}", | ||
| ] | ||
| ]); | ||
| } catch (ClientException $exception) { |
There was a problem hiding this comment.
Does this mean the issue with the given jira-key does not exist? If so we could report that for the comment (as it might be a typo) as a rule error
There was a problem hiding this comment.
According to API's response message, 404 means
Issue does not exist or you do not have permission to see it
So I'd say we shouldn't assume it's an error to be reported.
| $credentials = self::getCredentials($credentialsFilePath); | ||
|
|
||
| $this->credentials = base64_encode($credentials); | ||
| $this->client = new Client([ |
There was a problem hiding this comment.
can we utilize some kind of caching and/or cache revalidation?
I remember a dedicated jira api client which supports it, so we could do similar stuff I think.
I don't say we neeed the jira api client but maybe we can take inspiration from it. I am also fine with adopting the jira api client.. I like the jira client, as it does not depend on a http client package
wdyt?
There was a problem hiding this comment.
I wonder that maybe phpstan's cache is enough? I don't know exactly how phpstan's cache works but I assume that if it's not invalidated altogether for some reason (e.g. configuration change), then the same comment won't be analyzed twice if it doesn't change. This means that the API call is done only once.
If we wanted to use cache for Jira requests in our extension how do you think it should work? Should we introduce some parameter that developers could adjust to their needs (e.g. keep the status in cache for 5 minutes)? I tried to quickly analyze the client you mentioned but I don't see how it caches API calls. Can you please give me a hint where to look?
I like my dev tools to use as few dependencies as possible to minimize risks of conflicts, etc. Maybe it would be a good idea to not depend on jira client nor on guzzle and use low-level ext-curl and ext-json functions to make these calls? This should be quite simple to do and there would be less things to worry during the maintenance of the extension. If we add support in the future for more issue trackers, then using some client dependency for each of them could lead to quite complex dependency graph which isn't necessarily required for such a simple task as fetching a status from APIs. What do you think?
There was a problem hiding this comment.
Agree. As simple as possible would be great. Plain curl sounds good.
It seems I misremembered the cache thing with a client to a different api. I can't find it too :-)
|
👀 This seems like an interesting development. I’m currently using Youtrack. So once this PR is merged (or about to) I could check if I can add support for Youtrack in a new PR. |
Co-authored-by: Markus Staab <maggus.staab@googlemail.com>
| @@ -57,24 +57,24 @@ private static function decodeAndValidateResponse(string $body): array | |||
| $data = json_decode($body, true); | |||
|
|
|||
| if (!is_array($data) || !array_key_exists('fields', $data)) { | |||
| throw self::throwInvalidResponse(); | |||
There was a problem hiding this comment.
opened a phpstan issue on that one, as it should be detected as a error:
|
I think, it's ready to be merged now unless anyone has some more suggestions or see potential issues. I ran it on some of my projects and it works well. |
| continue; | ||
| } | ||
|
|
||
| $ticketStatus = $this->fetcher->fetchTicketStatus($ticketKey); |
There was a problem hiding this comment.
wdyt about prevent duplicate requests to the api, for the same key used in several comments?
|
just a few nits. will merge after that |
|
thank you - released in https://github.com/staabm/phpstan-todo-by/releases/tag/0.1.16 |
Hi!
First of all, thank you for this extension, because it allowed me to easily create something I needed for my personal project: to analyze codebase for todos with JIRA ticket keys and to make sure commented code has been fixed in the referenced tickets.
I noticed that your extension is getting more and more features so I decided to create this Pull Request with the changes I made to support my need to fetch statuses from JIRA. Maybe someone else is looking for such a feature and maybe you'll find it useful for your project.
I'm not sure though if it is expected from static analyzer to make API calls to some external provider but I made this rule optional (can be allowed with a flag in parameters). Currently only JIRA is supported but in the future this could also include other issue trackers. At the moment only basic authentication with Personal Access Token or user password is supported which was enough for my personal use case on local machine but most of organizations run their phpstan analysis in CI so if this PR is accepted, I'll work on changes to support OAuth tokens which are more appropriate for this use case.
If you would like to include this feature in your project, please feel free to share your comments and suggestions so I can work on this to match the vision you have for this extension.