Best way to deal with changes during code reviews

Development
1

@jcfr, @lassoan, @pieper I was just wondering what is the best way to integrate changes requested during a code review done in Github.

After accepting changes in Github your commits look like this (Approach 1):

Update qSlicerWidget.cxx
Update vtkMRMLApplicationlogic.cxx
ENH: add something in qSlicerWidget
ENH: add something in vtkMRMLApplicationLogic.cxx

Should I integrate the changes directly in the corresponding final commits? This leads to just (Approach 2):

ENH: add something in qSlicerWidget
ENH: add something in vtkMRMLApplicationLogic.cxx

here, we will lose the traceability of the code review.

Maybe I should leave the Update commits, but in order for you to squash them when you do the merge? (Approach 3)

Update qSlicerWidget.cxx
ENH: add something in qSlicerWidget
Update vtkMRMLApplicationlogic.cxx
ENH: add something in vtkMRMLApplicationLogic.cxx
2

Also. How about change requests that were not directly applied in Github? Should there be new commits for this, or perhaps the changes should happen directly in the corresponding PR commits?

3

You can submit all the changes as subsequent commits in the same PR. It makes it a bit easier to see what you changed in response to comments. When the changes are complex then sometimes we force-push to keep things simpler and avoid any potential merge conflicts.

When everything is fixed then we either ask you to squash the commits (especially if it is not trivial, for example you want to split the changes to a few commits), but typically we can just squash all the changes into a single commit before merging.

1 Like