Recent Features
| |||||||||||||
Creating a Proper Path for Clean-up PatchesCreating a Proper Path for Clean-up PatchesPosted May 29, 2008 6:01 UTC (Thu) by midg3t (subscriber, #30998)In reply to: Creating a Proper Path for Clean-up Patches by AnswerGuy Parent article: Getting the right kind of contributions
I think the point is that whitespace-only changes at any time are noise. Trying to understand annotated code (a la Keeping whitespace changes in a separate tree sounds OK on paper, but in practice it means someone has to spend all day resolving conflicts between code changes and whitespace changes. Not fun.
(Log in to post comments)
Creating a Proper Path for Clean-up Patches Posted May 29, 2008 8:38 UTC (Thu) by ekj (guest, #1524) [Link] Perhaps. But there's also the point that sometimes two patches: 1) This patch does NOTHING other than reformat function foo() 2) This patch adds a check for $ERROR_CONDITION to foo() Is easier to read and understand the patches than it would've been if they where rolled into one.
Don't do it then Posted May 29, 2008 9:24 UTC (Thu) by khim (subscriber, #9252) [Link] If reformatting of the patch is such a big part of it that it's hard to understand what the patch is actually doing - then drop the reformatting part. It's as simple as that. Formatting patches are bad. We once hired some guy and asked him to add small functionality to the program. It was supposed to be week or two of work. He took month and at the end we've got requested functionality and... patch which changes 90% of code to make it adhere to coding standards. It was the last month this guy worked for us: the work was redode by someone else. The rule of thumb is simple: you must be ready to answer for all lines your patch touches. You are "owner" of said lines after patch is applied (most VCS will allow you to find such owners). And answer like "I've only moved this two whitespaces right" does not count: you should genuinely understand this code. If you can do it - feel free to reformat code. If you don't have interest or time - leave the code as is!
Don't do it then Posted May 29, 2008 11:09 UTC (Thu) by ekj (guest, #1524) [Link] It's not that it's hard. It's just that -any- noise subtracts from readability. Which means a patch that -only- does the minimum required for adding the error-check will always be more readable than one that does that, plus fix the formatting of the surrounding 5 lines. So your advice adds up to, essentially: NEVER FIX STYLE-ISSUES. Which is overdoing it. I agree that it's *often* a bad thing to do style-change. But there's two important readability-issues here: One is the readability of the patch. That is always better with zero style-changes. The second is the readability of the code after the patch is applied. If that is sufficiently improved by doing style-fixes, those may be worthwhile even though it hurts the patch.
|
|||||||||||||