Add PR review checklist #1192

arjunsavel · 2020-06-18T21:34:25Z

Per hackathon 7/15 discussion, GH actions currently don't pass secrets necessary for bot comments to PRs made from forks. Will convert to draft for now, but hopefully there'll be some progress on this front w.r.t. GH actions functionality at some point!

Description

I'm adding a GitHub workflow file that provides a comment below each PR after it's opened.

Motivation and Context

This fixes #1169 by providing a checklist for reviewers to reference for every opened PR.

How Has This Been Tested?

I've run this successfully on one of my own repos.

Screenshots (if appropriate):

**Per hackathon 7/15 discussion, GH actions currently don't pass secrets necessary for bot comments to PRs made from forks. Will convert to draft for now, but hopefully there'll be some progress on this front w.r.t. GH actions functionality at some point!

Per hackathon 7/15 discussion, GH actions currently don't pass secrets necessary for bot comments to PRs made from forks. Will convert to draft for now, but hopefully there'll be some progress on this front w.r.t. GH actions functionality at some point!

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have assigned/requested two reviewers for this pull request.

codecov · 2020-06-18T21:48:39Z

Codecov Report

Merging #1192 into master will increase coverage by 0.58%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1192      +/-   ##
==========================================
+ Coverage   77.13%   77.71%   +0.58%     
==========================================
  Files          89       91       +2     
  Lines        5551     5727     +176     
==========================================
+ Hits         4282     4451     +169     
- Misses       1269     1276       +7

Impacted Files	Coverage Δ
tardis/io/util.py	`73.56% <0.00%> (ø)`
tardis/analysis.py	`0.00% <0.00%> (ø)`
tardis/io/decay.py	`89.65% <0.00%> (ø)`
tardis/util/base.py	`77.77% <0.00%> (ø)`
tardis/stats/base.py	`0.00% <0.00%> (ø)`
tardis/io/__init__.py	`100.00% <0.00%> (ø)`
tardis/plasma/base.py	`58.27% <0.00%> (ø)`
tardis/_astropy_init.py	`56.81% <0.00%> (ø)`
tardis/io/model_reader.py	`97.70% <0.00%> (ø)`
tardis/io/parsers/csvy.py	`89.74% <0.00%> (ø)`
... and 56 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e95ca26...f51b8a9. Read the comment docs.

arjunsavel · 2020-06-24T05:42:36Z

@harpolea — what items do you think would be sufficient for the PR checklist? I think I remember you discussing this, but can't quite remember!

harpolea · 2020-06-24T14:36:31Z

@arjunsavel You can find the list that we came up with in the TARDIS google drive. I'll paste it here as well:

Before a PR is accepted, it must meet the following criteria:

Is the necessary information provided?
- Does the PR have a complete description? Does it explain what the PR is attempting to do/fix, does it explain how it does this?
- Is there an explanation of why this PR is needed?
- Please use the following PR template: https://github.com/tardis-sn/tardis/blob/master/.github/PULL_REQUEST_TEMPLATE.md
Is this a duplicate PR?
- If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
- If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
Does it pass existing tests and are new tests provided if required?
- The test coverage should not decrease, and for new features should be at or very close to 100%.
Is the code properly documented?
- If this modifies existing code, then the docs should be updated. If this adds a new feature, additional documentation should be created.
- Sphinx and docstrings in the code (in numpydoc format)
Does this conform to PEP 8 and the TARDIS style guidelines?
- TODO: write TARDIS style guidelines
Does the PR fix the problem it describes?
- Make sure it doesn’t e.g. just fix the problem for a specific case
- Is this the best way of fixing the problem?
Is the code tidy?
- No unnecessary print lines or code comments

jaladh-singhal · 2020-06-30T20:35:40Z

.github/workflows/PR_reviews.yml

+                  [ ] If this modifies existing code, then the docs should be updated. If this adds a new feature, additional documentation should be created. \n
+                  [ ] Sphinx and docstrings in the code (in numpydoc format) \n
+                [ ] Does this conform to PEP 8 and the TARDIS style guidelines? \n


Where does "TARDIS style guidelines" live - can we add a link to them here?

I don't think that we have these yet — currently an issue (#1170 ) and a PR (#1183 ). Would you recommend removing this line for now, waiting until we have a style guide to push this, or something else?

Yeah I think we can remove TARDIS style guidelines and perhaps put black code style (with black hyperlinked).

arjunsavel · 2020-07-08T18:12:59Z

Also, the action is failing here, but it seems like this is a problem with GitHub Actions permissions for PRs from forks. This should be fixed once it's merged with master. I've gotten something similar working on one of my own repositories.

jaladh-singhal

@arjunsavel This bot look really good to me. 👍

But I think we need to talk (next Wednesday) about what things we want to add/remove/modify from this, considering ideas we dropped in PR template document today. Besides we also need to lighten up the content in TARDIS PR template since many things will be covered by this bot's checklist.

.github/workflows/PR_reviews.yml

jaladh-singhal · 2020-07-08T18:37:40Z

@epassaro @Jordi5 I would encourage you to look at this - if the bot brings up the checklist, IMO we can remove the checklist altogether from PR template which will help to not mistaken them as to-dos by GH and will also make PR template simpler as other examples we saw today. Let me know what do you guys think?

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>

arjunsavel · 2020-07-16T20:38:48Z

Per hackathon 7/15 discussion, GH actions currently don't pass secrets necessary for bot comments to PRs made from forks. Will convert to draft for now, but hopefully there'll be some progress on this front w.r.t. GH actions functionality at some point!

epassaro · 2021-01-23T11:19:05Z

@arjunsavel see https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target

marxwillia · 2021-05-24T18:08:23Z

@aribalam This would be a good PR for you to work on during the bonding period.

aribalam · 2021-05-25T05:24:38Z

@marxwillia Right on it!

Add template PR review comment

1155249

arjunsavel added the priority - low 🐌 label Jun 18, 2020

arjunsavel requested review from harpolea and jaladh-singhal June 18, 2020 21:34

arjunsavel assigned harpolea and jaladh-singhal Jun 18, 2020

Update with correct checklist

ca0c338

jaladh-singhal reviewed Jun 30, 2020

View reviewed changes

Indent correctly

72fafbe

arjunsavel marked this pull request as ready for review July 8, 2020 18:17

jaladh-singhal reviewed Jul 8, 2020

View reviewed changes

.github/workflows/PR_reviews.yml Outdated Show resolved Hide resolved

jaladh-singhal assigned epassaro and ebjordi Jul 8, 2020

Use hyperlink in PR comment

f51b8a9

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>

arjunsavel marked this pull request as draft July 16, 2020 20:38

wkerzendorf added the documentation label Oct 14, 2020

wkerzendorf force-pushed the master branch from e169e6c to 101a493 Compare November 17, 2020 20:55

aribalam mentioned this pull request May 25, 2021

Added PR review checklist workflow #1588

Merged

10 tasks

andrewfullard closed this Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PR review checklist #1192

Add PR review checklist #1192

arjunsavel commented Jun 18, 2020 •

edited by wkerzendorf

Loading

codecov bot commented Jun 18, 2020 •

edited

Loading

arjunsavel commented Jun 24, 2020

harpolea commented Jun 24, 2020

jaladh-singhal Jun 30, 2020

arjunsavel Jul 8, 2020

jaladh-singhal Jul 8, 2020

arjunsavel commented Jul 8, 2020

jaladh-singhal left a comment

jaladh-singhal commented Jul 8, 2020

arjunsavel commented Jul 16, 2020

epassaro commented Jan 23, 2021

marxwillia commented May 24, 2021

aribalam commented May 25, 2021

Add PR review checklist #1192

Add PR review checklist #1192

Conversation

arjunsavel commented Jun 18, 2020 • edited by wkerzendorf Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Jun 18, 2020 • edited Loading

Codecov Report

arjunsavel commented Jun 24, 2020

harpolea commented Jun 24, 2020

jaladh-singhal Jun 30, 2020

Choose a reason for hiding this comment

arjunsavel Jul 8, 2020

Choose a reason for hiding this comment

jaladh-singhal Jul 8, 2020

Choose a reason for hiding this comment

arjunsavel commented Jul 8, 2020

jaladh-singhal left a comment

Choose a reason for hiding this comment

jaladh-singhal commented Jul 8, 2020

arjunsavel commented Jul 16, 2020

epassaro commented Jan 23, 2021

marxwillia commented May 24, 2021

aribalam commented May 25, 2021

arjunsavel commented Jun 18, 2020 •

edited by wkerzendorf

Loading

codecov bot commented Jun 18, 2020 •

edited

Loading