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

[RFC]: .github: improve the PR template #33118

Merged
merged 1 commit into from Nov 9, 2021

Conversation

paper42
Copy link
Member

@paper42 paper42 commented Sep 25, 2021

Filling out the "Does it run and build successfully" section is
necessary only when the CI is skipped, but this is easy to miss.
Separate it from the rest of the text to make it clearer.

cc @ailiop-git

@ericonr
Copy link
Member

ericonr commented Sep 25, 2021

Want to couple this with not making them tick boxes anymore? Having all PRs with checklists is a bit bothersome.

@paper42 paper42 changed the title .github: clarify the PR template [RFC]: .github: improve the PR template Sep 25, 2021
@paper42
Copy link
Member Author

paper42 commented Sep 25, 2021

Want to couple this with not making them tick boxes anymore? Having all PRs with checklists is a bit bothersome.

I made a draft of how it could look. It needs a grammar review and we also need to decide if this is the right way to go. @void-linux/pkg-committers

.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
@Piraty
Copy link
Member

Piraty commented Sep 28, 2021

some of the usual suspects already miss the point of why this was added in the first place, i'm all for cleaning it up

@paper42 paper42 force-pushed the pr-template-clarify branch 2 times, most recently from a396af6 to 9524340 Compare October 3, 2021 21:52
Copy link
Member Author

@paper42 paper42 left a comment

Choose a reason for hiding this comment

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

I removed the options we don't want to encourage - "I did not test this PR" and "This package doesn't conform to the quality requirements".

Comment on lines 3 to 4
<!-- Note: PRs should only have one entry in the following section, otherwise
it will be assumed they have not been tested at all. -->
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this fine or is it too harsh?

Copy link
Member

Choose a reason for hiding this comment

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

Policy is just right, statement is kind of implied by previous one.

Copy link
Member

Choose a reason for hiding this comment

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

not harsh and to the point.

how about something like *YES*|*NO* (in bolds) to make it both (even more) obvious to contributor that an answer is required and obvious to maintainer if it wasn't answered (like not checking any checkbox before).

Copy link
Member

Choose a reason for hiding this comment

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

good idea of yes|no

Copy link
Member Author

Choose a reason for hiding this comment

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

Policy is just right, statement is kind of implied by previous one.
not harsh and to the point.

I think I will leave it there even though it's implied by the first comment just to be explicit.

how about something like YES|NO (in bolds) to make it both (even more) obvious to contributor that an answer is required and obvious to maintainer if it wasn't answered (like not checking any checkbox before).

Something like this?

#### Testing the changes
- I confirm this PR works for me: **YES**|**briefly**|**NO**

#### New package
- This new package conforms to the quality requirements: **YES**|**NO**

#### Local build testing
- I built this PR locally for my native architecture, (ARCH-LIBC)
- I built this PR locally for these architectures (if supported. mark crossbuilds):
  - aarch64-musl
  - armv7l
  - armv6l-musl

This way we can not differentiate between well tested and lightly tested PRs. The last section can not be adapted to this form, so it will be inconsistent with the rest of the PR template.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me.

  • Maybe link the quality requirements, so new committers can find them
  • Give committers instructions when it's okay to remove a statement and which are mandatory to open a PR.

Copy link
Member

Choose a reason for hiding this comment

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

Second thought: separate lines are easier to edit, thus better for permanent contributors.
Could that be changed to something keyboard-friendly like ** YES briefly NO **, at least?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing lines has an unfortunate consequence - it's not immediately obvious which option was selected compared to the current way (checkboxes). This might make PR template useless if we don't figure out how to clearly distinguish the options - different words at the start, different length, etc.

I would be in favor of the YES NO way because it's clear where to look when reviewing a PR.

Copy link
Member

Choose a reason for hiding this comment

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

Let's try to distinguish, with decoupling "tested" from "using" on top.

#### Testing the changes
- I regularly use the affected package
- Just contributing with package I use sporadically to never

- I tested the changes in this PR by using package with changes for some time
- Automated `do_check` passed, nothing can possibly go wrong

#### New package
- This new package conforms to the [quality requirements](https://github.com/void-linux/void-packages/blob/master/Manual.md#quality-requirements)
- Excuse to not conform to the quality requirements is ...

* switch from using checkboxes to **YES**|**NO** options
* make it clear that filling out the part about manual builds is only
  necessary when the CI is skipped, this was easy to miss
@Piraty
Copy link
Member

Piraty commented Nov 8, 2021

fine, everyone?

@Vaelatern
Copy link
Member

:shipit:

@Piraty Piraty merged commit 980cf96 into void-linux:master Nov 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2022
@paper42 paper42 deleted the pr-template-clarify branch April 8, 2022 11:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants