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

pull_request_template: Update and improve PR template. #1357

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

neiljp
Copy link
Collaborator

@neiljp neiljp commented Mar 26, 2023

What does this PR do, and why?

This adjusts the PR template to look roughly like in this Pull Request (small updates made later).

This was motivated by the testing section being somewhat redundant due to CI now testing for isolated commits, and grew from there.

The main changes are:

  • Simplifying titles contributors see while editing via ### instead of bold
  • Adding 'and why' to the 'What does this PR do?' top title
  • Updating the 'Tested?' section to be 'How did you test this?'
    • Generally clarifying each entry
    • Manual checks split into behavioral and visual entries
    • 'Linting and tests per commit' removed in favor of a refactor-only entry
  • Adding a self-review checklist (for each commit) to emphasize expectations
  • Adding an 'External discussion & connections' section with checkboxes
    • The comments re including Fixes and chat link requests are now here
    • The 'Interactions' section examples are also moved here
    • Other entries are added to prompt contributors (unmerged/followup PRs)
  • Adds tips to the Visual changes section
    • Zulip general tools
    • using asciinema for 'videos'

Outstanding aspect(s)

  • Is this section obvious enough in prompting what to fill in? (when viewed raw, not rendered)
  • Is it sufficiently obvious how to use the section below?
  • Do existing/new contributors both find these changes an improvement?
  • Is the self-review checklist useful?
  • Are the use of each set of checkboxes clear?

External discussion & connections

  • Discussed in #zulip-terminal in PR template feedback
  • Fully fixes #
  • Partially fixes issue #
  • Builds upon previous unmerged work in PR #
  • Is a follow-up to work in PR github: Add pull request template. #1071
  • Requires merge of PR #
  • Merge will enable work on #

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior
  • Existing automated tests should cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

The main changes are:
- Simplifying titles contributors see while editing via ### instead of bold
- Adding 'and why' to the 'What does this PR do?' top title
- Updating the 'Tested?' section to be 'How did you test this?'
  - Generally clarifying each entry
  - Manual checks split into behavioral and visual entries
  - 'Linting and tests per commit' removed in favor of a refactor-only entry
- Adding a self-review checklist (for each commit) to emphasize expectations
- Adding an 'External discussion & connections' section with checkboxes
  - The comments re including Fixes and chat link requests are here
  - The 'Interactions' section examples are also moved here
  - Other entries are added to prompt contributors (unmerged/followup PRs)
- Adds tips to the Visual changes section
  - Zulip general tools
  - using asciinema for 'videos'
@neiljp neiljp added the area: infrastructure Project infrastructure label Mar 26, 2023
@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Mar 26, 2023
@neiljp neiljp merged commit eacf627 into zulip:main Mar 27, 2023
@neiljp
Copy link
Collaborator Author

neiljp commented Mar 27, 2023

I merged this after a little feedback; I think this may get more indirect feedback by how it ends up getting used. #1360 filed to track points to consider, initially from above aspects.

@neiljp neiljp added this to the Next Release milestone Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: infrastructure Project infrastructure size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants