2024-04-12 - Do you know how to write a great Pull Request? - Chewing the Fat 📈

[sethdaily]

Hey SSWers,

Let's talk about these related rules...

main rule (where the comments are shown) - https://www.ssw.com.au/rules/write-a-good-pull-request/
https://www.ssw.com.au/rules/use-pull-request-templates-to-communicate-expectations/

What do you think about how you do PRs?
Do you have any PR tips?

[Freego1783]

PR templates are a good idea, I'd like to see them on every project I work on for consistancy! I feel like it's nice especially when you're a new developer on a project.

[tiagov8]

Dealing with GitHub as editor and reviewer in the past years made me naturally enhance my PRs and Issues to include the most information as possible.

There is a huge gap between the knowledge (on the purpose of a PR) from the person who created a PR; and the reviewer who is 1st looking at it. That gap can be reduced with great titles and descriptions.

[tkapa]

PR templates are quite nice.

A well structured PR with minimal change is ideal. When a change starts to become > 10 files there might be a problem.

[ncn-ssw]

The authoring of properly written PRs needs to be strictly enforced by team members to ensure that future team members understand why changes were made.

[ozdemir-mehmet-cer]

CER Templates are quite good. Who wrote them, someone from SSW?

[calumjs]

PR templates make sense, but I don't like the way ours begins with the tip before it gets to the actual change:

Tip: Use SSW Rule Writer GPT for help with writing rules 🤖

When I get a notification about a PR I don't need to see a tip about using SSW Rule Writer GPT intended for the person who wrote the PR every time.
Maybe we could put it at the end? Or include instructions to delete it in the template?

[jaydenalchin]

I completely agree 🫡 #8356

[AshAnil]

PR Templates are good to follow for consistency. It is also easier for the approver to understand why the PR was made.

[jakebayliss]

I like to add in bad code to see if Calum actually reviews my PR's. He always gets them @calumjs
PR templates are a great idea - I think manually writing them just takes too long, especially if they are in good detail. I think there is a good in between with too much and too little

[chrisschultzssw]

I see a lot of pull requests that start with "XS✔ ◾ ".

While (I think) the meaning is self-explanatory, I don't see this in any of our PR rules (or in the links, e.g. https://gitmoji.dev/)...

[bahjatalaadelssw]

I liked the points in the video, but I use most of these techniques in file management hierarchies for footage and assets

[danielmackay]

Adding images or screenshots to PRs, helps to add helps more context for the reviewer.

Also, using it's super easy to review the entire branch for a PR in GitHub by pressing 'dot' on your keyboard.

Rider, has also just released GitHub PR integration so you can review and comment on all changes right inside of your favourite IDE 😎

[matt-goldman]

Absolutely agree. Before and after screenshots or even a done video are awesome in a PR.

[Dhruv-0987]

This is amazing, had no idea. I am never gonna review a PR on GitHub or DevOps anymore

[GordonBeeming]

I love PR templates. If you don't feel aligned with your PR template for your project, you should talk to your team. Often its not great for everyone, but nobody wants to start the conversation and everyone is using it in anger

[cvionnet]

Having a template for PRs makes the process smoother and more consistent. This should be implemented for every project.

[YalalCakirSSW]

I like the idea of having templates for PRs. I think every team should have a template to achieve consistency. It would be great to see TimePro using one 👀

[matt-goldman]

PR templates are cool but there are other things we can focus on to help improve merge debt.

[jerryluossw]

Great rules, I believe the PRtemplate is very useful.
However, I wonder if we need 1. What triggered this change? (PBI link, Email Subject, conversation + reason, etc), because PR is associated with PBI or Work Item in Azure DevOps, the information should be include in PBI or Work Item.

[jernejk]

I think the CTF was great! :)

While I like having PR templates, the "1. What triggered this change? (PBI link, Email Subject, conversation + reason, etc)" suggest that it's expected to not follow Scrum rules and PR needs to reference untrackable tasks. Ideally, PR on it's own is already associated to the correct GitHub issue, making this question irrelevant.

"2. What was changed?" is ok. I think "2. What is the impact?" is much better as it explicitly states what would be most useful for a PR review. While it's easy to see what changed (just look at changed files), it's difficult to determine how this PR impacts the code base. Do we have a new feature, does the bug fix add a new validation, will the user see a new input on the UI?

In short, templates are OK, but I think it would be nicer for them to be a bit more explicit in what we want and remove first question if it's not relevant. In Azure DevOps, we can't complete a PR without linking it to a PBI or bug which directly references what triggered the change.

[Aibono1225]

I like the PR template on Rules.Content, it always gives me useful guidance when I create the PR.

[bettybondoc]

I've been taught to make my commits small and my descriptions long. Well, not literally long but detailed - by providing enough context for my team and my future self. Six months down the line, even I might not remember the specifics of the changes I made so this approach ensures that when we look back at the history, we'll have a clear understanding of what was done and why.

[tino-liu]

All well written PR description gives the reviewer context before they look at the code. I want to first understand what is being done before I look into the micro details on how it is implemented. Great rule!

[PennyWalker]

Templates are great. The way I often use GitHub doesn't really make the template relevant to me because it's a basic upload of an image, but I try to put as much of the template in that makes sense

[SofieHong]

The CTF and the PR template was great 🌟 Especially for enhancing transparency and consistency

[zl24622]

Great CTF this week. I learned the PR templates and liked them. The tips from the video are also helpful.

[leahy268]

The PR Template is good. The video was awful though. I mean I agreed with the content but it took a long time to get there.

[andrewwaltosssw]

PR templates are a great tool. They are meant to make PRs faster to raise, more meaningful and consistent. It's important to make sure that the template is created with those ideals in mind.

[vladislav-kir]

PR templates are a great idea, since it might force the people, who would not normally do it, write a good description.

I did not like the video though, it could have been significantly shorter without loosing any important content....

[JackDevAU]

PR templates are cool! They help give a lot more context.

Figure: A Famous programmer once said. LGTM

[babakamyljanovssw]

I like PR templates, it makes the process consistent among team members

[MarcusIrmscherSSW]

My PR's are usually fairly simple but its still helpful having a template to start with.

[ozair-ashfaque]

PR templates are essential, they keep consistency and help the reviewer to give the context for the changes.