-
Notifications
You must be signed in to change notification settings - Fork 573
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] Pull Requests Template #3496
Comments
Congratulations on your first Artsy RFC 🙌
|
Here is an example from an OSS project that Artsy uses: https:/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! |
@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 |
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. 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:
As an example of (3), here is Eigen's git log:
and here is the git log for one of the tools from my last place:
|
Sorry, forgot to say: 👍 on the RFC! |
@adamvert Mmm! I can't dispute those logs 😍 But I worry about what might get lost. Specifically:
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.) |
@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... :) |
@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? |
@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!) |
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? |
@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 👍 |
One that worked well for me and a previous team was something like the following. The main ideas were
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: IntroductionThe type of this PR is: eg. Enhancement
DescriptionImplementation descriptionTest PlanChecklist
Labels
For the reviewersCopy-paste this in a comment below, post it, and start checking things. Reviewer Comment
|
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:
|
Nice RFC @MounirDhahri! Overall, I'm in favor of a template to establish consistency between PRs! 👍 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:
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. |
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 DescriptionTest 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) |
@MounirDhahri sounds great! Here are the steps to close an RFC: https:/artsy/README/blob/master/playbooks/rfcs.md#resolution |
ResolutionAll new PRs will now be following the agreed on PR template Level of Support2: Positive feedback. Additional Context:PR templates will for now only be adopted in Eigen Next StepsWe will implement it. ExceptionsNo exceptions so far |
Proposal:
Using Github Pull Request Templates on Eigen. I propose the following template inspired by awesome-github-templates
Types of changes.
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:
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.
The text was updated successfully, but these errors were encountered: