search

psss / did

What did you do last week, month, year?
https://did.readthedocs.io/
GNU General Public License v2.0
246 stars 104 forks source link

[WIP] :gift_heart: Listing Merged PRs for Github and Gitlab #311

Open cardil opened 12 months ago

cardil commented 12 months ago

Changes

psss commented 12 months ago

Thanks for the pull request. Actually, the PullRequestsClosed stats are intended for listing merged pull requests:

* Pull requests closed on github: 9
    * teemtee/tmt#2221 - Fix test checking custom destination for libraries
    * teemtee/tmt#2216 - Create plans to cover individual step features
    * teemtee/tmt#2206 - Group discover/fmf options, improve wording a bit
    * teemtee/tmt#2203 - Add template option to polarion report
    * teemtee/tmt#2178 - Add cache_property for things that are generated...
    * teemtee/tmt#2173 - Simplify public git conversion with a declarative list
    * teemtee/tmt#2172 - Clean up logging in `tmt.utils.create_directory()`
    * teemtee/tmt#2171 - Spec-based container becomes generic over input...
    * teemtee/tmt#2160 - Move test framework code into distinct framework...

* Pull requests reviewed on github: 6
    * teemtee/tmt#2203 - Add template option to polarion report
    * teemtee/tmt#2178 - Add cache_property for things that are generated...
    * teemtee/tmt#2173 - Simplify public git conversion with a declarative list
    * teemtee/tmt#2172 - Clean up logging in `tmt.utils.create_directory()`
    * teemtee/tmt#2171 - Spec-based container becomes generic over input...
    * teemtee/tmt#2160 - Move test framework code into distinct framework...

* Merged pull requests on github: 3
    * teemtee/tmt#2221 - Fix test checking custom destination for libraries
    * teemtee/tmt#2216 - Create plans to cover individual step features
    * teemtee/tmt#2206 - Group discover/fmf options, improve wording a bit

Not sure how exactly the query author:{0}+merged:{1}..{2} works but definitely some of the pull requests merged by me during this week are not listed. See above.

2uasimojo commented 8 months ago

I'll say that I don't understand the logic behind "closed". When I use --github-pull-requests-closed the list doesn't seem to include any pull requests I authored. What I want to see (non-exclusively) is pull requests I worked on (authored or co-authored) that have merged. If this PR makes that possible, I'm all for it.

cardil commented 8 months ago

PR can be closed because of being merged or declined. This takes just the successful PRs.

psss commented 6 months ago

I'll say that I don't understand the logic behind "closed". When I use --github-pull-requests-closed the list doesn't seem to include any pull requests I authored. What I want to see (non-exclusively) is pull requests I worked on (authored or co-authored) that have merged. If this PR makes that possible, I'm all for it.

Pull requests authored by a user are covered by pull-requests-created. The intention behind the pull-requests-closed stat was to give the credit to the person who took care of merging the pull request. This can cover making sure that tests passed and whatever action or process is agreed in the project. Such person should be set as the pull request assignee. Do you propose a different approach?

PR can be closed because of being merged or declined. This takes just the successful PRs.

Understood. It might make sense to separate closed (without merge) and merged pull requests. However, we need to make sure the stat is working as expected. As mentioned in the comment above, some of the pull requests merged by me in the given time frame are not reported by the new stat. Could you please, have a look into that?

2uasimojo commented 6 months ago

Hi @psss!

Pull requests authored by a user are covered by pull-requests-created. The intention behind the pull-requests-closed stat was to give the credit to the person who took care of merging the pull request. This can cover making sure that tests passed and whatever action or process is agreed in the project. Such person should be set as the pull request assignee. Do you propose a different approach?

I think of the assignee as the reviewer*. I dig the idea of did having a knob to report all the PRs I reviewed. Useful subsets would be:

--pull-requests-created is really great... but it would be really useful to see a similar breakdown for those, which I think was the intention behind this PR.

Having worked on CLIs for more than 20y, I appreciate that you may not want to maintain a massive proliferation of flags. That said, I could envision the following non-mutually-exclusive options that could be used in conjunction with all/most of the --github-* ones:

...where the default is --open --merged --abandoned

I haven't thought through whether/how these could also apply to --jira-*, but it could be similar.

*I imagine different shops use different processes, so this may not apply universally, but this is how it's done at Red Hat, at least in the OpenShift org, so I know it's not just me :)

psss commented 6 months ago

I think of the assignee as the reviewer*.

Well, for reviewers there's a dedicated field, right? I would not suggest to duplicate the meaning.

  • PR is still open (my "review queue"), possibly reporting how many reviews I've already left on it.

Just to clarify, the existing pull-requests-reviewed stat does not cover this use case? Unfortunately, the api for reviewed pull reuqests does not always produce correct results (reported the issue several years ago but no progress on that). So I'm not sure how much this can be improved.

  • PR was closed because merged.

+1 for having pull-requests-merged

  • PR was closed without merge.

Your suggested name pull-requests-abandoned makes it less ambiguous, so +1 for this as well.

2uasimojo commented 6 months ago

I think of the assignee as the reviewer*.

Well, for reviewers there's a dedicated field, right? I would not suggest to duplicate the meaning.

Mm, fair point. The difference being that any schmo can leave a review, whereas in our process the assignee's reviews are the "important" ones -- specifically the ones that the author expects to move the PR toward getting merged. I guess the subtleties of different processes come into play here... but empirically having separate flags for "assigned" vs "reviewed" PRs would give users the flexibility to report in whatever way is most appropriate for them.

  • PR is still open (my "review queue"), possibly reporting how many reviews I've already left on it.

Just to clarify, the existing pull-requests-reviewed stat does not cover this use case?

If I could further filter by open/abandoned/merged, I would think so, yes.

cardil commented 6 months ago

@psss As mentioned in the https://github.com/psss/did/pull/311#issuecomment-1653296183, some of the pull requests merged by me in the given time frame are not reported by the new stat. Could you please, have a look into that?

This is straightforward. The -merged is reporting only the PRs you have authored/created, and then it was merged.

The -closed also lists the PRs authored/created by other people, you have approved. That's different.

I need the merged PR (authored by me), as I report the code I, personally, contributed.

psss commented 6 months ago

I need the merged PR (authored by me), as I report the code I, personally, contributed.

Isn't the credit for authoring a pull request covered by pull-requests-created? What is the work you want to track if you don't review and don't merge the pull request? I would say the contribution of the author is to create the pull request.

cardil commented 6 months ago

@psss Isn't the credit for authoring a pull request covered by pull-requests-created? What is the work you want to track if you don't review and don't merge the pull request? I would say the contribution of the author is to create the pull request.

The created, might not be merged. So, created contain 3 types: ongoing PRs, successful, and dropped. I just want successful ones, which I've created - merged. Also, the time is important. Creation time != merge time. I want the merge time.