On Mon, Nov 15, 2010 at 11:41 PM, Luke Kanies <[email protected]> wrote: > I think the exact opposite should be true - accept cleanup and refactoring > patches wherever and whenever we can. Heck, I think we should give extra > credit for refactoring patches that change no behaviour at all. > As to whether all of his changes meet our standards, his points about our > standards not being published is a good point, and I think it makes sense to > use his patch as a forcing function to publish what bits are not yet > decided.
Paul and I spent a little while looking at this refactor, and in the end we agree the changes are good, but it was REALLY hard to tell that from the diff. This was because code was moved around, changed, and some non-refactoring work (changing exception raised) was all done in the same commit. I've updated the development lifecycle document to specify that refactor commits should be done in separate commits to improve diff readability. Sandor, I created an example of how we think the changes you made could have been done as a couple commits. https://github.com/mmrobins/puppet/tree/ticket/next/5079 I didn't make the exception rename change since that wasn't a refactor, or move "Default" out to a constant since that was a refactor that currently isn't useful. If you'd like to change your patch to make the commits clearer and resubmit we'd appreciate that, or if you like we can just merge in my branch as we've reworked it. Then you could make the exception change as a separate commit if you still think it's useful. Matt -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
