Skip to content

Add PR Links as Task List Items to Issues #518

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Nov 13, 2022

Conversation

saadmk11
Copy link
Contributor

@saadmk11 saadmk11 commented Nov 7, 2022

closes #517

@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #518 (2dd66ef) into main (be82c42) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #518   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines         1998      2034   +36     
  Branches       227       231    +4     
=========================================
+ Hits          1998      2034   +36     
Flag Coverage Δ
Python_3.10.8 100.00% <100.00%> (ø)
Python_3.11.0 100.00% <100.00%> (ø)
Python_3.8.14 100.00% <100.00%> (ø)
Python_3.9.15 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
bedevere/gh_issue.py 100.00% <100.00%> (ø)
bedevere/util.py 100.00% <100.00%> (ø)
tests/test_gh_issue.py 100.00% <100.00%> (ø)
tests/test_util.py 100.00% <100.00%> (ø)

@saadmk11 saadmk11 marked this pull request as draft November 7, 2022 18:54
# If the body already contains a legacy closing tag
# (e.g. <!-- /gh-pr-number -->), then we use the legacy template
# TODO: Remove this when all the open issues using the legacy tag are closed
if PR_BODY_CLOSING_TAG.format(tag_type=PR) in body:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am leaving the issues updated with the old implementation (e.g. <!-- /gh-pr-number -->) alone.

@saadmk11 saadmk11 marked this pull request as ready for review November 8, 2022 10:56
saadmk11 and others added 2 commits November 11, 2022 17:01
Co-authored-by: Ezio Melotti <[email protected]>
Co-authored-by: Ezio Melotti <[email protected]>
@ezio-melotti ezio-melotti merged commit 4abd68d into python:main Nov 13, 2022
@saadmk11 saadmk11 deleted the add-task-list branch November 13, 2022 16:11
@ezio-melotti
Copy link
Member

This seems to work (at least with a single linked PR): python/cpython#99443
We'll see if it works with multiple PRs too, but so far it looks good.

Thanks again @saadmk11!

@saadmk11
Copy link
Contributor Author

Hi @ezio-melotti, Looks like this is not working for multiple PRs (python/cpython#96192). After some investigation I found that tasklists doesn't just render the new UI, it also re-writes the Issue body.

Example:

<!-- gh-linked-prs -->

https://github.com/python/cpython/issues/96192#tasklist-block-8fd60492-131e-4b4a-abab-06800aa4bb11

<!-- /gh-linked-prs -->


<!-- gh-linked-prs -->

https://github.com/python/cpython/issues/96192#tasklist-block-ea9583fe-18a8-47ee-aec9-08a62427146b

<!-- /gh-linked-prs -->

I found this after I inspected the API response for python/cpython#96192 (https://api.github.com/repos/python/cpython/issues/96192). This did not happen for the repositories I tested this PR on because it did not have this feature enabled.

This re-write causes multiple issues for us:

  1. Important As you can see from the API response above the re-write even removes the reference to the linked PR (gh-{pr-number}). This causes the bot to add the same PR multiple times to the same issue (os.path.ismount() doesn't properly use byte-paths from an os.DirEntry cpython#96192) and we have no way to check what PR was added before.
  2. The RegEx does not match and we can not be sure if the re-write behavior will remain after this feature out of beta. (All the other type of rendering features does not re-write issue body)

I would suggest switching to checklists at least for now until tasklists are out of beta. As I do not have access to any repository with this feature enabled I was not able to investigate any further.

@ezio-melotti
Copy link
Member

FWIW this is the relevant part of the body of the issue linked above, but as you noticed, this is different from what is received from the API.


<!-- gh-linked-prs -->

```[tasklist]
### Linked PRs
- [x] https://github.com/python/cpython/pull/96194
```


<!-- /gh-linked-prs -->


<!-- gh-linked-prs -->

```[tasklist]
### Linked PRs
- [x] https://github.com/python/cpython/pull/96194
```


<!-- /gh-linked-prs -->


<!-- gh-linked-prs -->

```[tasklist]
### Linked PRs
- [ ] https://github.com/python/cpython/pull/99455
```


<!-- /gh-linked-prs -->

we have no way to check what PR was added before.

The gh-XXXXX expansion seems straightforward -- can't you just look for https://github.com/python/cpython/(?:pull|issue)/\d+ (or even (?:https://github.com/python/cpython/(?:pull|issue)/|gh-)\d+) instead?

I would suggest switching to checklists at least for now until tasklists are out of beta.

This is definitely an option if the above suggestion fails.

As I do not have access to any repository with this feature enabled I was not able to investigate any further.

You can join the waitlist to request tasklists being enabled on one of your repos here: https://github.com/features/issues/signup

I also contacted GitHub for feedback, so if the suggestion doesn't work we can switch to checklist, and reconsider tasklists if/when they offer a solution.

@saadmk11
Copy link
Contributor Author

we have no way to check what PR was added before.

The gh-XXXXX expansion seems straightforward -- can't you just look for https://github.com/python/cpython/(?:pull|issue)/\d+ (or even (?:https://github.com/python/cpython/(?:pull|issue)/|gh-)\d+) instead?

Not sure what you mean here. The API returns this: https://github.com/python/cpython/issues/96192#tasklist-block-8fd60492-131e-4b4a-abab-06800aa4bb11. Here the URL is not the URL of the pull request but the URL of the issue the API is requesting.

As I do not have access to any repository with this feature enabled I was not able to investigate any further.

You can join the waitlist to request tasklists being enabled on one of your repos here: https://github.com/features/issues/signup

Unfortunately you can't join the wait-list as an individual user, only GitHub organizations are accepted.

@ezio-melotti
Copy link
Member

Here the URL is not the URL of the pull request but the URL of the issue the API is requesting.

I see, I thought it was the full URL for the PRs.

In that case, let's just switch to checklists for now.

@saadmk11
Copy link
Contributor Author

saadmk11 commented Nov 15, 2022

In that case, let's just switch to checklists for now.

Seems like auto marking a checklist items as completed (e.g: - [x] gh-170) when a PR is merged does not work, It only works if the checklist item is a GitHub Issues. @ezio-melotti Should we still proceed with this?

@ezio-melotti
Copy link
Member

Seems like auto marking a checklist items as completed (e.g: - [x] gh-170) when a PR is merged does not work

If that's the case (i.e. merging a PR doesn't mark it as completed in the checklist), then there is no advantage in using checklists over plain lists. The difference between checklists and plain lists is that the checklist shows the number of "tasks" at the top of the issue (as (1 of 3 tasks)) and that the PR will show up as Tracked in #XXX, however the former is useless if merged PRs are not marked as completed, and the latter is already covered by bedevere adding the issue link to each PR.

IOW, it's probably better to go back to plain lists for the time being.

@omerbensaadon
Copy link

Hey folks, we're investigating this as a bug on our end. So sorry for the delay here, we'll try and get this fixed in the new year and making sure multiple PRs work in these blocks.

Again, so sorry you all had to wait this long for a response and thank you so much for being Alpha (now Beta!) users :)

@CAM-Gerlach
Copy link
Member

Thanks @omerbensaadon ! Looking forward to it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve formatting of PR links added to issues
4 participants