Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

What did you do with all the comments at the end of the review?


If they are in there own commits, you just drop them.

Honestly the average level of git skills, doesn't allow such a workflow.


No, no git magic. Just source code changes. One commit with the whole review that contains fixes and maybe a few questions. Then a commit from the PR author on top of that that adds explanations and/or fixes for the questions.

Maybe the key required for this is that you have reviews where, if code prompts a question, then more often then not the code should change. At the very least to explain briefly why this approach was chosen over another approach.

If you have code reviews from developers with differing skill levels and the response to the question is just an explanations how the language or framework works then this might become a bit cumbersome (because you have to leave the comment to reply and then remove the whole discussion before merging)


> No, no git magic. Just source code changes. One commit with the whole review that contains fixes and maybe a few questions. Then a commit from the PR author on top of that that adds explanations and/or fixes for the questions.

Reviews on some projects will ask you to make a commit/patch in between your third and fourth (out of seven). And to fix a typo in the fifth commit message. And do fix something in the second commit that leads to conflicts in the next commits. And then to end up with for example seven commits that each “do one thing” or whatever. So if the expectation is that you end up with one “fixed up“ commit at the end of the review process which encapsulates the whole PR then this won't work.

With your approach I imagine going in several rounds where

1. Reviewers leave comments

2. I address them

3. I publish that as review round X

4. I “clean up“ the history (get rid of comments/all review history) and address things like “make a doc commit before commit number 4”

5. Publish that as the “blank slate” (no comments) for the next round

6. Invite the reviewers to the next round

And that could work.

Something more dynamic/less rigorous (with rounds) and you might end up with an excessive amount of history rewriting and conflict resolution.


I think your approach to code review is in general a good idea but don't underestimate the differences in skill level in the average project. I often work with domain experts who are not developers or beginners who are still learning basic git concepts and how to navigate the code base.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: