# Best way to deal with changes during code reviews

**URL:** <https://discourse.slicer.org/t/best-way-to-deal-with-changes-during-code-reviews/15446>\
**Category:** Development\
**Created:** [January 11, 2021, 8:21am UTC](https://discourse.slicer.org/t/best-way-to-deal-with-changes-during-code-reviews/15446 "2021-01-11T08:21:52Z")\
**Posts on this page:** 3\
**Page:** 1

<div class="post-metadata">

**Author:** ![RafaelPalomar](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/rafaelpalomar/32/1436_2.png) [@RafaelPalomar](https://discourse.slicer.org/u/RafaelPalomar)\
**Post date:** [January 11, 2021, 8:21am UTC](https://discourse.slicer.org/t/best-way-to-deal-with-changes-during-code-reviews/15446/1 "2021-01-11T08:21:52Z")

</div>

@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** ):

```auto
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** ):

```auto
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** )

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

```

---

<div class="post-metadata">

**Author:** ![RafaelPalomar](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/rafaelpalomar/32/1436_2.png) [@RafaelPalomar](https://discourse.slicer.org/u/RafaelPalomar)\
**Post date:** [January 11, 2021, 9:51am UTC](https://discourse.slicer.org/t/best-way-to-deal-with-changes-during-code-reviews/15446/2 "2021-01-11T09:51:33Z")

</div>

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?

---

<div class="post-metadata">

**Author:** ![lassoan](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/lassoan/32/13_2.png) [@lassoan](https://discourse.slicer.org/u/lassoan)\
**Post date:** [January 11, 2021, 8:01pm UTC](https://discourse.slicer.org/t/best-way-to-deal-with-changes-during-code-reviews/15446/3 "2021-01-11T20:01:29Z")

</div>

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.