Closed psteinb closed 6 years ago
yes I am fine with it, thanks for the fixing :)
travis shows green light ... @tdd11235813 can you review & merge please?
I applied the change. Feel free to do a squash and merge. I am personally more in favor of more atomic commits than monolithic commits. Atomic commits (1 feature, 1 file) support bisecting problems better, I believe.
you are right, it is better to keep commits as small as possible to ease bug identification. Commits also should represent a feature or entity (commits as a unit of change), where such a commit can span over multiple files. I also think of code refactoring as an example (renaming or changing a recurrent pattern). The advantage would be, that there is a more clean history and features are easy to identify and to revert (and rebasing/merging might become easier).
A commit should be self-contained and in a compilable state, where possible (otherwise it should be mentioned in the description with e.g. WIP). For example, the last 3 commits above are dependent and represent one feature, so it should be one commit. The other commits seem to be standalone, so they could remain.
Conclusion: Squashing all into one commit can be too coarse, and creating commits on a per-file mapping can make too much noise, i.e., the unit of change is not visible, neither in the PR nor in the commit level.
For big commits I keep my local develop branch verbose and create a PR with a cleaned up branch. Of course, it is not long-term safe, because other developers might do not have access to this verbose branch anymore.
This would be a good compromise in my opinion, how do you think?
feel free to squash-merge
merged (squashed last 3 commits). Thanks!
@tdd11235813 commit 97e5964 fixes a bug in the cmake setup of the tests, is it ok to keep it inside this PR?