this function basically checks if the value is Nan and returns a boolean.
I think it's not the best example. First, if it was really “basically”, you wouldn't need to write a comment. Second, the comment merely repeats the function name itself.
Better example would be something like: “I'm using the isNaN function from Lodash, which I've never used before, and I'm not sure it's the right way to use it”.
Also if it's something really tricky or unusual, better to write a comment in the code, not in the pull request.
If you think the feedback is valid, implement it! However big or small the change is, if you think the feedback is valid, always reply and let the reviewer know that you took their feedback.
As a reviewer I don't completely agree with this. Receiving 20 comments with “fixed” for a particular pull request isn't fun. As a reviews I expect that all comments will be addressed in some way: by changing the code or explaining why it won't be done. Saying something like “thank you, great idea” occasionally is a great thing though ;-)
Make your commit message obvious
I think this depends on a workflow and a particular team. Other all commits get squashed into one before merging, so only one commit message per pull request matters. Which should be very good ;-) Also as a reviewer I don't really care about particular commits, but maybe that's just me. For me a comment by the author, saying that the pull request is ready for another review iteration, is much more useful.
For further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
Great article Ankita!
I have couple of comments 🐿
I think it's not the best example. First, if it was really “basically”, you wouldn't need to write a comment. Second, the comment merely repeats the function name itself.
Better example would be something like: “I'm using the
isNaNfunction from Lodash, which I've never used before, and I'm not sure it's the right way to use it”.Also if it's something really tricky or unusual, better to write a comment in the code, not in the pull request.
As a reviewer I don't completely agree with this. Receiving 20 comments with “fixed” for a particular pull request isn't fun. As a reviews I expect that all comments will be addressed in some way: by changing the code or explaining why it won't be done. Saying something like “thank you, great idea” occasionally is a great thing though ;-)
I think this depends on a workflow and a particular team. Other all commits get squashed into one before merging, so only one commit message per pull request matters. Which should be very good ;-) Also as a reviewer I don't really care about particular commits, but maybe that's just me. For me a comment by the author, saying that the pull request is ready for another review iteration, is much more useful.