The main questions are:
- Should I refactor before every commit?
- Should a commit pass all tests?
- What units of code should be code-reviewed?
It got me thinking about my current approach and about my "ideal world" approach.
I'm a big fan of the idea of continuous deployment. It means that every commit, after passing all tests, can be automatically deployed.
I'm not there yet. I try to work with very short commits, often it's only 2-line changes. However, I still miss an infrastructure which could automatically push this commit, test it and deploy. It's not that it's difficult to prepare, it's just some kind of a psychological bareer. It's definitely something I want to improve as soon as possible.
Sometimes it's hard to have deployable commits. Recently we've been upgrading our app to Rails 3.1 and it's not something that can be done in one commit.
Going back to questions:
Should I refactor before every commit?
Refactoring is part of a workflow, but I wouldn't say it should part of every commit. Sometimes a commit can be just the red-green part, and it's the next commit that does the "refactor" part.
Should a commit pass all tests?
This is a tricky one. In my ideal world - yes, because it should be deployable. Is it hard to achieve in practice? I'm wondering if there's a conflict between a deployable commit and one that fails on some tests. Maybe it's fine to have some tests failing, if they are not part of the code that is being used in production?
What units of code should be code-reviewed?
We tend to make code reviews using GitHub commit comments. They're fine, but sometimes I comment on something that was changed 3 commits later, which makes my comment irrelevant.
Obviously it all depends on a project. In most cases I think we shouldn't be too serious with the code reviews comments. In my opinion code reviews should help us triggering some coding/architecture discussions, they shouldn't be too formal. I don't like proceses which require code reviews before the code goes into production - we should trust each other that nothing stupid gets deployed. If we treat a code review as a tool for better communication then it's not that important if a comment is about a code that changed later or not.
How about you? How often do you commit?
8 comments:
not every commit has to be deployable, i'm working on my repo so pulling from it (to the main repo) will be done when things will be ready. it's just my opinion. i find comfortable that i can see the changes from master repo perspective to someone elses repo and decide if it is ready or not.
Should a commit pass all tests? Definitely. Otherwise, it will mess your colleague's work when they do the update -- they won't know if the the test failed because of what they did or what you did.
Should I refactor before every commit? Definitely (at least IMHO). Code that barely passes the tests is just a half-product. A sort of prototype. Something that works, but you wouldn't show it to your colleagues, because it's ugly, repetitive etc. I wouldn't want somebody else to see it or comment on it, while I'm still working on it. For me it's just unfinished.
Also, when you have something like metric_fu installed on your continuous integreation, non-refactored commit will likely fail the build. And you get to wear the stupid hat :)
As often as possible - my main reasoning being that having a finer granularity of changes lets me revert a commit much easier in case of something going awry.
We've experimented with pre-commit code reviews with SVN a while ago, and that has worked okay. Our process at the moment uses reviewboard with a branch-per-feature approach - eg. a feature is started, reviewed and finished within a branch, and only then merged into a branch that is later deployed.
Should I refactor before every commit? Definitely not, IMHO. :-)
I prefer that refactoring is separated out into its own commit. Then others can clearly see which changes are behaviour-preserving, as opposed to actually adding or modifying functionality.
I think the urge to keep work private because it isn't finished (e.g. isn't refactored) should be resisted. It's better to be public and transparent about what you're up to. The sooner others see your changes, and the sooner CI gets to run against them, the better for everyone.
Should a commit pass all tests?
I've settled on a style where every commit is deployable, and passes all tests. Sometimes this means labelling the in-progress functional test as 'expected failure'. (Python unittest has a decorator for this, that ignores the failure in test results.) Martin Fowler's book 'Refactoring' describes how to do this in the trickier situations, such as when making a sweeping change that affect lots of code.
However, some of my current team-mates don't believe this is possible all the time, and for those times they continue to use feature branches to keep the non-deployable code isolated until it can be made working, then merged with the main development branch. I plan to pair with them on the next such change, to discover which of us is right.
very interesting points of view, i don't know how about You all but i fell that we all think about something else while saying "commit", at least what this "commit" will lead to (i.e. if team is working on one repository or distributed repos), as a consequence we are writing about something slightly different - still it's very interesting read :-)
We review code before each commit (even css-related stuff) and we like it, taking a constructive criticism easily has never been a problem to anyone in our team.
Keep writing :)
Thanks for all comments!
First, a clarification - when I talked about a commit, I meant a git-like commit, however it's not really that different in this context from Subversion-like comment.
@poulwiel:
I like the idea of having a quick feedback. That's why i love TDD, that's also why I like continuous deployment.
@szeryf
When you have a single commit broken (in git) it doesn't break your friend build.
Good point with metric_fu.
@jasiek
I hate feature branches, too complicated for my small brain :)
You're right about the possibility of reverting single commits.
@Tartley
You, sir, must come from a very good school of software development :) I agree with everything you wrote. I'm sure your team-mates will gain a lot when you show them how to refactor with small commits, being green all the time. That's how I learnt it. Pairing with someone who can do that makes miracles.
@ncr
Reviewing code before a commit may work fine in case of pair programming. In other cases the feedback is too slow and I hope you're not blocking anybody from commiting their changes. I can't see the advantages of such processes.
Criticisism is fine :) We sometime cross the line and go into haters-mode, but that's fine. I hate ugly code :)
Post a Comment