Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

simplify pull request template #219

Merged
merged 3 commits into from
Oct 29, 2022
Merged

simplify pull request template #219

merged 3 commits into from
Oct 29, 2022

Conversation

jameslamb
Copy link
Contributor

This PR proposes some simplifications to the pull request template.

I believe it'll make contributing a bit less intimidating for beginners, and reduce the amount of boilerplate text that has to be read by maintainers reviewing PRs.

Changes

I'll leave inline comments explaining each proposed change.

Testing

N/A

Notes

N/A

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.
  • Reviewers requested with the Reviewers tool ➡️

Comment on lines 5 to 10
-

## Testing

1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm proposing replacing this + Testing checklist with a section simply titled How I tested this.

I think having two sections that are both asking "how did you test this?" is confusing, as is having a checklist of two specific Python versions (how do I test on those versions? all the tests (e.g. Dask), or just the main ones?

Having a checklist of specific versions also requires some maintenance effort as the range of support Python versions for the project changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep thanks -- I was thinking that myself the other day that I was repeating myself.

Copy link
Collaborator

@skrawcz skrawcz Oct 28, 2022

Choose a reason for hiding this comment

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

we can always add back asking about versions if we need to, so this sounds good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jameslamb on second thought, thoughts on putting the testing section here versus at the end? I think my natural flow would be to list changes, and then list how I tested them? Thoughts? Otherwise the rest of this looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok yeah, that works for me! I don't have a strong preference about the ordering.

Just pushed moving testing up.

So the order looks like this now: 0333362

image

@jameslamb
Copy link
Contributor Author

Ok I'm done adding notes. Thanks very much for considering this!

Copy link
Collaborator

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

🚢 🇮🇹

@skrawcz skrawcz merged commit eb30e77 into stitchfix:main Oct 29, 2022
@jameslamb jameslamb deleted the simplify-pr-template branch October 29, 2022 22:06
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

2 participants