Skip to content
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

Update PR template #3153

Merged
merged 2 commits into from
Oct 18, 2023
Merged

Update PR template #3153

merged 2 commits into from
Oct 18, 2023

Conversation

JDBetteridge
Copy link
Member

So that it no longer looks like this:

---
name: "Bug fix or new feature"
description: ''
title: ''
labels: ''
assignees: ''

---

# Description
Please include a summary of the changes introduced by this PR.
Additionally include any relevant motivation and context. List any
dependencies that are required for this change, other PRs upon
which this may depend, and associated issues.

## Associated Pull Requests:
- Make a list (with links!)
- [Like this PR#691](https://github.com/OP2/PyOP2/pull/691)

## Fixes Issues:
- Make a list of issues (with links!)
- [Like this Issue#2824](https://github.com/firedrakeproject/firedrake/issues/2824)

If issues are fixed by this PR, prepend each of them with the word
"fixes", so they are automatically closed when this PR is merged. For
example "fixes #123, fixes #456".

# Checklist for author:

<!--
If you think an option is not relevant to your PR, do not delete it but use ~strikethrough formating on it~. This helps keeping track of the entire list.
-->

- [ ] I have linted the codebase (`make lint` in the `firedrake` source directory).
- [ ] My changes generate no new warnings.
- [ ] All of my functions and classes have appropriate docstrings.
- [ ] I have commented my code where its purpose may be unclear.
- [ ] I have included and updated any relevant documentation.
- [ ] Documentation builds locally (`make linkcheck; make html; make latexpdf` in `firedrake/docs` directory)
- [ ] I have added tests specific to the issues fixed in this PR.
- [ ] I have added tests that exercise the new functionality I have introduced
- [ ] Tests pass locally (`pytest tests` in the `firedrake` source directory) (useful, but not essential if you don't have suitable hardware).
- [ ] I have performed a self-review of my own code using the below guidelines.

# Checklist for reviewer:

- [ ] Docstrings present
- [ ] New tests present
- [ ] Code correctly commented
- [ ] No bad "code smells"
- [ ] No issues in parallel
- [ ] No CI issues (excessive parallelism/memory usage/time/warnings generated)
- [ ] Upstream/dependent branches and PRs are ready

Feel free to add reviewers if you know there is someone who is already aware of this work.

Please open this PR initially as a draft and mark as ready for review once CI tests are passing.

<!--
Thanks for contributing!
-->

@ReubenHill
Copy link
Contributor

I thought the aim was just to get rid of the

---
name: "Bug fix or new feature"
description: ''
title: ''
labels: ''
assignees: ''

---

bit?

Copy link
Contributor

@connorjward connorjward left a comment

Choose a reason for hiding this comment

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

I think putting all this into a comment is the right way to go. Perhaps the checklists should go into a policy (example)?

assignees: ''

---

# Description
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Description

Is this line necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea of having some sort of prompt. I'd really like to have it as a form, but this is not available on github

@JDBetteridge
Copy link
Member Author

@ReubenHill it is! See the diff

@ReubenHill
Copy link
Contributor

@ReubenHill it is! See the diff

But the diff gets rid of all the tickboxes etc too

@JDBetteridge
Copy link
Member Author

I know, but nobody was using them. My original intent was to add this so that essentially anyone could do a code review by following the steps and people who were unsure of what to do could just follow the steps.

I still like the idea, but if it's not being used we should probably get rid of it. A lot of PRs just deleted all the text anyway 🤷

@ReubenHill
Copy link
Contributor

I know, but nobody was using them. My original intent was to add this so that essentially anyone could do a code review by following the steps and people who were unsure of what to do could just follow the steps.

I still like the idea, but if it's not being used we should probably get rid of it. A lot of PRs just deleted all the text anyway 🤷

I used it...

@JDBetteridge
Copy link
Member Author

I know, and so did a couple of other people, but sadly you were in a minority. What I'd really like is a bunch of PR templates that you could select from like you can with issue templates, but it seems like this feature hasn't landed in github's interface yet. I have set up a folder with other templates for when they do enable it though!

@JDBetteridge JDBetteridge marked this pull request as ready for review October 12, 2023 11:04
- [ ] No CI issues (excessive parallelism/memory usage/time/warnings generated)
- [ ] Upstream/dependent branches and PRs are ready
If issues are fixed by this PR, include link to them and prepend each of them with the word "fixes", so they are automatically closed when this PR is merged.
For example "fixes #123, fixes #456"..
Copy link
Contributor

Choose a reason for hiding this comment

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

This (because no-one deletes the text) links every PR for all eternity to these two issues. fixes #xyz ?

@dham dham merged commit b231609 into master Oct 18, 2023
7 of 8 checks passed
@dham dham deleted the JDBetteridge/update_PR_template branch October 18, 2023 15:48
Ig-dolci pushed a commit that referenced this pull request Oct 20, 2023
* Update PR template

---------

Co-authored-by: David A. Ham <david.ham@imperial.ac.uk>
pbrubeck pushed a commit that referenced this pull request Oct 25, 2023
* Update PR template

---------

Co-authored-by: David A. Ham <david.ham@imperial.ac.uk>
pbrubeck pushed a commit that referenced this pull request Oct 25, 2023
* Update PR template

---------

Co-authored-by: David A. Ham <david.ham@imperial.ac.uk>
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.

5 participants