Closed anttihaap closed 6 years ago
I'd also consider doing some clean up on the commit history, the 20+ "new try n" commit messages especially are not too helpful :) Discuss this with @nygrenh to determine what -- if anything -- should be done.
About the number of commits: I concur with @ljleppan, this pull request shouldn't have this many commits. This doesn't mean I'm expecting you to squash your commits into a single one. Something like 5-10 commits would be appropriate for a pull request of this size.
You can merge your commits with a simple interactive rebase operation. Please see https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History for more details.
@nygrenh and @ljleppan: All the comments should now be addressed, so from our point of view the request seems ready for merging. Let us know if there is something more to consider, though! (Note that the comments about newlines as well as the advised change to RPlugin in the very last comment should be also shown as outdated on this github interface, i.e. the changes are implemented)
LGTM, but I'll refer the decision to merge to @nygrenh. Well done :)
:tada:
Coverage increased (+0.5%) to 73.78% when pulling 73723c8217d8fbc4d234185714ee59ae58c02958 on RTMC:master into 3e34e49e0aee7c5fe17267c0f13258c5416cb620 on testmycode:master.