Please, Respect The Guy Who Will Review Your Pull Request
Just a short article after I got a meager pull request.
This is an article not related to Javascript. But I got a pull request a few days ago. This pull request belongs to a JS project. It inspires me to write this article. I use “your pull request” instead of “your code” in the title because I won’t talk much about code in this article. I’ll focus on some rules showing reviewers you respect them.
I’ll make an example of a terrible pull request I got here. In this article, we will talk about some things around this pull request.
Let’s go to the first rule.
Readable, readable, and readable!
I repeated this work three times because It’s really an important thing. I saw some developers create pull requests that aren’t readable. When I say “readable” in this part, It’s a metaphor. “readable” is a pure readable. Let’s talk about a pull request I saw last week as an example. Let’s see again a picture of the pull request with some highlights.
Firstly, we may see that the title is a broken sentence. GitHub will fill the title automatically for us when we create a new pull request. In the case, our branch name or last commit message is too long, GitHub may reduce us like the image above. I know it. But I also know that GitHub only prefills the title, we can edit it. But it seems the guy who make the pull request never read it any time after he created it. The result is a pull request with the title “Fix: fixed the problem when press back button but the router is not u…”. This is a typical case of non-readable pull requests. As a normal reader, I really can’t read this title because It’s not a complete sentence.
As I said above, the title may prefill by GitHub from the last commit or branch name. I tried to read the last commit and I find the full version of the title. But it can’t remove an image of a careless developer in my mind. In my career, I saw many pull requests like this. Besides a reason from the carelessness of developers. I also have another reason that is subjective. They don’t put readable content into the pull request because they believe the reviewer can understand it. We may face that case many times in our life. It happens when someone sends their pull request to others with a summary in another channel (eg: the chat message). But somehow we got this pull request (of course, without the context).
So I think in all of the cases, we should make a pull request with readable content.
Understandable
After the first thing that’s also an essential thing. We will move to this thing — understandable. I write this part from the things I was trained in my old company. Honestly, it’s one best thing I learned in my career. Again, we will follow the example from the previous part. We just need to assume that the title from this pull request is “Fix: fixed the problem when press back button but the router is not updated and the modal still here”.
In this case, I think all of the contents in this pull request are readable. But I’m not sure about their “understandable”. I’ll talk about why I think that.
When I look at the pull request content, I only saw the list of changes. Honestly, I can see it when I read the files changed tab without this list. Back to the pull request description, besides a list of changes that are written meagerly. I don’t see any other information like the reason or target of these changes. I understand I may completely try to find them if I focus to read the code changes. But honestly, I don’t think it is a good choice. Because it’s a waste of time. In the good case, I understand right the target of the developer who created the pull request. It still takes too much time than I read it from the pull request description. In the bad case, I understand wrong the idea from the creator, this guy will need to spend time to explain for me about this pull request. Of course, this case also is solved by writing a clear pull request description.
Besides, saving the time of reading and understanding code changes, writing an understandable description may help us avoid some problems. Besides the target and the solution, we also provide other things from these changes like the context of these changes (eg: bug ticket…). It helps us avoid the risk in the future. Because we may have a good view when we review a pull request. Sometimes, we get some pull requests which seem reasonable. But the problem is this change is only reasonable in a special case. When we try to zoom out of this place and view the project with a larger view, we may realize that this change may break other places. It’s really danger if we don’t make this check.
So this is a little conclusion of this part. Personally, I really feel the respect from the pull request creator, if this guy is writing an understandable description. That’s the reason. Firstly, It shows me that a creator does this task carefully and seriously. Secondly, It tells me that a pull request creator treasures my time, if he/she gives me a pull request have a reason and a solution clearly. The last reason is I really like a pull request to have a clear context. It shows me that a creator doesn’t try to push the puck to me.
Don’t have any feedback been ignored
The last part is also an extra part about some bad actions. In our life, our pull requests aren’t always reviewed by a guy who has a level higher than us. I think it’s a normal thing. Everyone can review others' codes. Of course, they also leave feedback when they see any things aren’t good in our pull requests. But I saw some developers ignore other feedback in some contexts. One of the most popular contexts is the reviewer just asking a small question (it’s not a request change). We have many reasons for these questions. It may be that the reviewer isn’t sure about the code they think should optimize. Or, just a suggestion to make your code better but it isn’t required. For me, we shouldn’t ignore this feedback for any reason. Maybe it’s unnecessary or stupid feedback. But please explain to them your thinking. Because when you request someone to review your pull request, I think you should be ready to get feedback from them. Maybe the guy who gives you feedback isn’t the guy who you request review. But again, you shouldn’t ignore anyone. Please respect others and their feedback.
Conclusion
There is what I want to share today. Just very small notes that are may show reviewers see we respect them. Besides the rules above, I have a solid rule. Let’s put ourselves to other views. Let’s imagine that you are the guy who will review your pull request.
Thanks for your reading.
