[RFC] Pull Requests Template

MounirDhahri commented 4 years ago

Proposal:

Using Github Pull Request Templates on Eigen. I propose the following template inspired by awesome-github-templates

Types of changes.

[ ] Bugfix (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)
[ ] Documentation Update (if none of the other choices apply)
[ ] Other: (Please describe)

Proposed changes

Describe what does this PR implement/fix.

Ticket/Issue Link: (If available)

Screenshots

…

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

Reasoning

Providing descriptions for pull requests is highly recommended for a better code review process. This will help with:

What can reviewers expect while reviewing the submitted code?
What should reviewers focus the most on?

We currently have no standard pull request template in Eigen and maybe it's time to formalize Pull Requests among developers.

We want to avoid having different description formats for PRs and to have only one polished PR description format to make the review process easier.

How is this RFC resolved?

We decide if we will be using PR templates and if yes, choose a template.

iskounen commented 4 years ago

Congratulations on your first Artsy RFC 🙌

Are there any templates used by other open source projects use that we can use as a reference?
To keep it simple at he beginning, I propose having 1 template with all of the sections (description, screenshot, JIRA link, etc.) that we like to see in PRs regardless of the type of change being made. It can be up to the engineers to pick and choose which sections their PR needs.

ashfurrow commented 4 years ago

Here is an example from an OSS project that Artsy uses: https://github.com/fastlane/fastlane/blob/master/.github/PULL_REQUEST_TEMPLATE.md

I'm 👍 on this RFC. I'm not sure that the "types of changes" section is strictly necessary. But I would also be okay trying it out to see how it goes, too!

MounirDhahri commented 4 years ago

@iskounen I updated the RFC description to include some pull request templates.

@ashfurrow After a second thought, yes it may not be necessary since we'll be including a description

admbtlr commented 4 years ago

Related to the "types of changes" section: I'm a huge fan of semantic commit messages where the type of change is part of the commit message, e.g. feat: add hat wobble, fix: stop hat falling off, docs: describe hat wobble, chore: add config for hat wobble deployment, etc.

At the last place I worked, we had a strict policy of squashing commits when merging a PR, so we only enforced semantic messaging on the PR title (which then becomes the description of the single, squashed commit). It takes a bit of getting used to, but it has two main advantages:

You can immediately understand the scope of open PRs, just by scanning the titles
Enforces modality: you end up thinking twice before you hide a bugfix inside a new feature
Beautiful git logs!

As an example of (3), here is Eigen's git log:

*   9bd6b5411 (HEAD -> master, origin/master, origin/beta, origin/HEAD) Update metaphysics schema (#3508)
|\
| * 84e5d6739 Update metaphysics schema
|/
* e6c8fd1e8 (origin/app_store_submission) Update dep @artsy/cohesion from 1.13.0 to v1.14.0
*   d15e46204 Removes Unused Metaphysics v1 Queries #trivial (#3501)
|\
| *   c6ff3a77a Merge branch 'master' into metaphysics-v1-cleanup
| |\
| |/
|/|
* |   a455697d0 [MX-269] Delete Unused Files (#3504)
|\ \
| * \   f8e6219f9 Merge branch 'master' into delete-unused-files
| |\ \
| |/ /
|/| |
* | |   e0e2fa427 Update image for nightly job to use macOS #trivial (#3500)
|\ \ \
| * \ \   a8742cac9 Merge branch 'master' into agree-to-terms
| |\ \ \
| |/ / /

and here is the git log for one of the tools from my last place:

* eefed57c (HEAD -> develop, origin/develop, origin/HEAD) refactor: move producer to aether.producer (#865)
* 50c08b42 docs: typo (#866)
* f457fd2c test: set dynamic priority to task execution (#862)
* 0e873fe4 chore: LOGGING_LEVEL and named volumes in docker (#861)
* f24a49bf fix(odk): xml choices (#859)
* 51b0468b chore: upgrade to python 3.8 (#855)
* ca60bf72 feat(kernel): input from AVRO schema endpoint (#857)
* 65cf97a7 test: assertRaises check exception outside with (#854)
* 29ea0c3f chore: wrong internal AETHER_KERNEL_URL
* fbea9c74 feat: performance tests (#849)

admbtlr commented 4 years ago

Sorry, forgot to say: 👍 on the RFC!

ashfurrow commented 4 years ago

@adamvert Mmm! I can't dispute those logs 😍 But I worry about what might get lost. Specifically:

you end up thinking twice before you hide a bugfix inside a new feature

What's nice about having non-semantic and non-squashed commits is that it allows/encourages us to clean up the code as we go. I always put the fix in its own commit, because I do appreciate a precise log history – but I personally tolerate the messy logs because of the tradeoff of constantly-improving code seems worth it to me. Other teams/projects do have their own approaches, though – ideally we could trial out something like semantic commits on a smaller repo as a testbed 👍 (And automate this with Danger or Peril.)

admbtlr commented 4 years ago

@ashfurrow Totally +100 on constantly improving the codebase! But I find that things are easier to manage if you put the bugfix in a separate PR. IMHO a PR should be granular, and do one thing. That thing might be a feature, a bug fix, a doc update, etc. But I find that it's much easier to reason about things if the bugfix is done entirely separately from the feature, and lives forever in the easily parsable log.

Admittedly it makes development slightly more complicated, in that you sometimes have to back out of the branch you're working on in order to create a new branch for a one line bugfix (and then maybe if you need the bugfix, merge that branch into the feature branch you're working on before the PR itself gets merged into master). But I feel like this is how git is actually supposed to work: it encourages a kind of fluidity in the practise, but a solidity in the resultant artefacts (i.e. codebase and logs).

Also, there's nothing I ❤️ more than one line PRs! But maybe that's just me... :)

ashfurrow commented 4 years ago

@adamvert That makes sense! It's for sure a balance. Can you share any ideas on encouraging engineers to do that extra work in git, to still encourage improving things as you go, in the context of strict git/PR policies?

admbtlr commented 4 years ago

@ashfurrow That's a good question. I guess there's a certain amount of inverse "broken windows"-type effect, whereby a constant level of attention to the state of the repo and codebase encourages everyone to increase their personal level of attention: take pride in your repo! celebrate your log!

Also, if you're using semantic commits, it's very easy to see e.g. how many fixes people have been doing, which means you can have "bugfix champions", or like "chore champions": people who are keeping dependencies up to date. The transparency is pretty motivational, IMHO.

(Of course there's a danger of this becoming a bit big brother, surveillance-y, but that comes down to culture, and I really can't imagine that happening at Artsy, from what I've seen so far!)

admbtlr commented 4 years ago

Of course this has now gone way beyond the context of @MounirDhahri 's very good RFC that initiated the discussion, so maybe I should write a separate RFC on the larger question of git best practices?

ashfurrow commented 4 years ago

@adamvert I think an RFC might be a good way to test the waters on how people are feeling about it (see this example of an open-ended RFC discussion). If you're looking for a less structured and synchronous discussion, I can recommend using a Lunch & Learn slot on Thursdays to discuss as well 👍

pvinis commented 4 years ago

One that worked well for me and a previous team was something like the following. The main ideas were

a high-level description of the change and a more technical implementation explanation,
a test plan for the creator or the reviewer,
a checklist of things to check as a reviewer. The thing I liked the most was the first bullet, then the second, then the third 😅.

I'd just like to say we should keep it simple, at least for the first version. If it was completely up to me I would just add a high-level desc, an implementation explanation, and a test plan. Easy and quick to write/list/read/understand/test.

Take a look:

Introduction

The type of this PR is:

eg. Enhancement

This PR resolves #

Description

Implementation description

Test Plan

Checklist

[ ] Code quality is high (good comments, no unneeded changes, good names)
[ ] Unit tests are added to test behavior

Labels
T: type of PR Bug-fix, Chore, Enhancement
F: feature, so that related PRs can easily be grouped FeatureName
D: relevant platform Android, iOS
Native: if this PR requires a native release Android, iOS
S: external deps Waiting for backend, Waiting for translations
Helpers WIP, Direct PR
[ ] Labels are added

For the reviewers

Copy-paste this in a comment below, post it, and start checking things.

Reviewer Comment

``` ### Review - [ ] I understand the purpose of this PR - [ ] I understand the code of this PR - [ ] Target branch is correct(`master`, `staging`) - [ ] Works correctly on iOS - [ ] Works correctly on Android - [ ] The code quality is high (proper place, proper structure, good names) - [ ] There's enough documentation to explain what is going on #### Just before merging - [ ] All my requested changes have been resolved ```

ashfurrow commented 4 years ago

keep it simple, at least for the first version

Yeah I like this idea quite a bit. I would say even simpler would be a good idea, especially since we use tooling like Peril/Danger to test for things like "have you added unit tests?"

How about something like:

# Introduction

The type of this PR is:

eg. Enhancement

This PR resolves #

Here's what it looks like:

<!-- add screenshots if applicable -->

## Description

Implementation description

## Test Plan

<!-- if necessary -->

dblandin commented 4 years ago

Nice RFC @MounirDhahri!

Overall, I'm in favor of a template to establish consistency between PRs! :+1:

I'll echo what has already been suggested that we keep it simple to start and lean on what authors tend to include today without a template.

Looking over recent PRs in artsy/eigen the common includes seem to be:

Description
Links to Jira tickets
CI tags like #trivial or #skip_new_tests
Screenshots / Simulator Recordings

One specific suggestion I have is to move the screenshots / recordings section to the bottom of the template as those media embeds tend to take up quite a bit of vertical space.

I don't know how applicable this is, but the discussion around types of commits and PRs between breaking changes, enhancements, bug fixes, and trivial updates reminds me of of the flow suggested by the intuit/auto tool. I've really enjoyed that automation for artsy/reaction (deprecated) and artsy/palette (:tophat: tip to @zephraph for championing that).

It does a great job of bumping the version number based on PR labels, automating changelog entries, and managing releases in GitHub. I wonder if this tool could be used with our Fastlane setup. It might be interesting to have auto manage versions using GitHub labels and fastlane to handle deployment to Testflight and the App Store.

MounirDhahri commented 4 years ago

I agree with @dblandin, I prefer having the screenshots at the end.

It seems like what we all agree on is something simple close to this format:

The type of this PR is: Enhancement

This PR resolves: MX-10000

Description

Test Plan (if necessary)

Screenshots / Simulator Recordings

@dblandin About the automation using intuit/auto tool, that would be a nice idea. I think we should investigate it for the future. About the Fastlane integration, there is actually a new plugin for that fastlane-plugin-semantic-release based on conventional commits (instead of semantic commits)

ashfurrow commented 4 years ago

@MounirDhahri sounds great! Here are the steps to close an RFC: https://github.com/artsy/README/blob/master/playbooks/rfcs.md#resolution

MounirDhahri commented 4 years ago

Resolution

All new PRs will now be following the agreed on PR template

Level of Support

2: Positive feedback.

Additional Context:

PR templates will for now only be adopted in Eigen

Next Steps

We will implement it.

Exceptions

No exceptions so far

artsy / eigen