Preface

Author Mark Seemann, 10-tips-for-better-pull-requests, 15 January 2015 This is a thought of the article (10-tips-for-better-pull-requests) written by Mark Seemann.

1. Make it samll

Personal thought: Agree with the point. First, too many files in one PR will take too long to review, which also means many differences. If the requirement is complicated to split or the PR includes other requirements in it. And this issue depends on how you break down a need. The ability to break down depends on the scope, your familiarity, the kind of solution you choose, and existing architect limitations. By the way, the deadline can break everything.

2. Do only one thing

Personal thought: This point is the extension of thought of point 1. The PR can make smaller when you focus on one issue. Same as I agree with the first point, this one is worth adopting. This one can also help you refine the thought when you commit it. But in reality, sometimes we will reformat the files in passing. We can put them in one commit; that will be fine. These files should be relative to the file with the requirement.

3. Watch your line width

Personal thought: In general, this one can adopt, but be aware that do not sacrifice the readability of the code (ex, using rarely used abbreviations.); that will be to put the cart before the horse. Another relative issue will be the description of the commit title; make it short and precise. Here is for your reference. This article is worth reading!

4. Avoid re-formatting

Personal thought: Hummm…This approach can make PR and commit clean, without a doubt. But it needs time to achieve; when team members do not use to do it will panic about it. So I accept having this kind of commitment in one PR. Of course, you should put all of these in one commitment. Doing this makes life easier, and we also have a clean PR.

5. Make sure the code builds

Personal thought: Well-said! Every commit should follow this rule. This rule is similar to ‘The boy scout rule’ in the book Clean Code. It would be best if you made a code change to ensure it gets better. Stop the code small getting worse.

6. Make sure all tests pass

Personal thought: Totally agree with it—no new error before you send a PR out. The existing test should not have the error, of course.

7. Add tests

Personal thought: This one is nice to have in the present day (2021). It is still nice even if you do it after you finish the requirement. Some teams can follow this rule strictly. But nothing can go against the low of ‘Time to market.’ it’s hard to convince the manager you will slip the schedule because you want to add tests. Usually, the developer will run locally to verify whether complete the requirement or not (this approach usually saves time when you test this only several times, not in the long term). Another reason people don’t do this is because it needs to be listed first. Also, we usually sacrifice this first because we don’t have to do this. In the workplace, we have a role named QA, who will be responsible for code quality (no need to write a unit test, but take responsibility for quality).

8. Document your reasoning

Personal thought:

About ‘code comments are apologies.’ here is a short part of the post:

A comment is an apology for not choosing a more clear name, or a more reasonable set of parameters, or for the failure to use explanatory variables and explanatory functions. Apologies for making the code unmaintainable, apologies for not using well-known algorithms, apologies for writing ‘clever’ code, apologies for not having a good version control system, apologies for not having finished the job of writing the code, or for leaving vulnerabilities or flaws in the code, apologies for hand-optimizing C code in ugly ways. And documentation comments are no better. In fact, I have my doubts about docstrings.
by Uncle Bob

Honest speaking, uncle Bob follows a rigorous rule.

I have yet to see people spend time on the commit message in my career life. Here is a good example of an excellent commit message. So you will know why this one tip is willing to adopt. You can quickly see the cause and effect that makes the code change. Here is a great way to write a readable commit message; please reference conventional commits and this post.

PR comment depends on the team, except for the open-source project. It will be great if the team cloud uses the PR management system (ex., GitHub Pull Request) well. Then, the team can focus on the code change and why it makes it, which helps to figure out the context of the change. However, as I told you before, most teams do not use a PR management system to manage it. Instead, most teams used to discuss in a task-dispatch system (ex., JIRA, Trello, Asana, etc.).

9. Write well

Personal thought: This one is more like preaching, not being pedantic in the comment. Do your best to ensure the words are clear, simple, and make sense. I can’t agree more; we spent most of our time reading, not writing.

10. Avoid thrashing

Personal thought: I seldom see people make commit messages in order (or make them more straightforward, sample, and readable). For those who are doing this, a kind of cleanliness. But the commit message will do more valuable than before. And I rarely use rebase to rewrite a commit message, only if thrashing happened. I usually use this command to reduce merge commits; I prefer seeing my PR have fewer merge commits. Because the merge commit has no extra value to your PR, I use rebase to sync to the main branch to reduce the merge commit. That’s back to the arrangement of the commit message. It takes time, especially if you have a deadline to complete the mission. I have often seen that we sacrifice unit tests and anything helpful on readable at the beginning of the project. It’s hard to persuade the manager to do things that do not have business value, especially if you are running out of time.

Reference