Update code review guidelines #683
Conversation
On my current client project we have been using labels as a prefix to comments we leave on code reviews to clearly display intention and tone. This removes a lot of ambiguity from written text which can be interpreted in many ways. Our usage of labels has been informed by this article https://conventionalcomments.org/. This PR would bring this optional way of working to other people within thoughtbot.
|
This sounds interesting. I am not sure if I have misunderstood intent in a PR. I guess I assume positive intent. That said, I'd be open to giving this a try! Note to self: This would be a good point to re-watch, "Implementing a Strong Code-Review Culture", by Derek Prior |
|
I think it's a nice suggestion to have, but I'm not a fan of it myself. I generally dislike things that make me sound like a robot when I'm writing and so if I had a series of little things (like the nitpick example), I like to write how I'm thinking at the time and that's often going to change. Not only that, but I like to think of reviewing code as much as a conversation as possible and if that means I have to write a lot more to express myself then I'm okay with that. |
|
@smaboshe and @nickcharlton thank you so much for your feedback on this! |
|
I haven't used this to convey tone, but I'm a fan of using |
I'm OK with this phrasing, as it gives everyone the option to use this system, or not. I am also way more conversational and verbose in all types of communication, so I may embed the fact that it's a curiosity, non-blocking, or a nit, in the sentence, rather than the label. Before we merge it into our guides, it could be good to hear a broader spectrum of opinions, but then again, you @samithoughtbot shared about this PR in our slack channels and whatnot, to get reviews from folks. See how verbose I am? |
|
Thank you all for your feedback on this!
I am going to trust my best judgement and merge this in. If this was a brand new point that was not optional, I don't think I would merge this in. Hopefully, this will at least provide visibility to an optional practice that may help others. |
On my current client project we have been using labels as a prefix to comments we leave on code reviews to clearly display intention and tone. This removes a lot of ambiguity from written text which can be interpreted in many ways.
Our usage of labels has been informed by this article https://conventionalcomments.org/.
This PR would bring this optional way of working to other people within thoughtbot.