4 ms·

We have a rule that you can only prettify (to defined style rules) when you are already making a change to that piece of code or file. Otherwise it's hand off

by junto 11y ago

We have a rule that you can only prettify (to defined style rules) when you are already making a change to that piece of code or file. Otherwise it's hand off as a rule. It also stops programmers wasting time.

jacquesm 11y ago
That's a great way to do it: fix-as-you-go. The same goes for adding unit tests where those are missing (in bulk, rather than as a set going with a recently touched piece of code), fixing up documentation issues and other global changes such as naming conventions and so on. Hitting all of the code base in one go with a thing like that is asking for trouble. Downvoters are cordially invited to state why they disagree with this.
- junto 11y ago
  I work on a team maintaining a large code base. We follow a similar rule concerning unit tests. Our testing over 5 years has slowly grown from 0% to give us ~75% coverage, by using the methodology you describe.
err4nt 11y ago
I'll offer another opinion on this - I an a frontend developer and so it's very common for me to have to deal with files that are written in PHP, HTML, JavaScript, and also sometimes including CSS embedded right in it. Roughly speaking, PHP and JavaScript have a similar syntax and can be formatted/styled in a similar way, but HTML it a totally different beast and has very different structure, as well as CSS which is a glorified list of rules. I don't refactor code for the sake of it, however, in an effort to read related files I often clean things up that I must read in order to edit another section. I don't discard my cleanups because I'm leaving it better than when I found it (and cleaning up a messy repository is tough, but a little in each commit can help). So I agree that you should style where you're working - but there are valid reasons to refactor or restyle code in other places too. Here's an example of a file I've been updating lately: - before: http://i.imgur.com/X37RFQD.png http://i.imgur.com/X37RFQD.png - after: http://i.imgur.com/9cjiJxn.png http://i.imgur.com/9cjiJxn.png
- jacquesm 11y ago
  There are exceptions to every rule, and yours is a valid exception to the rule that in general one should not commit non-functional change-sets. If something is particularly ugly then you could clean it up but that change should not be taken for granted not to have any adverse effects, there is really no such thing as an 'innocent' change. I've seen whitespace changes break code far too frequently to not be suspicious of any change at all.
dalore 11y ago
I believe that's the wrong way to do it. You can only prettify/reformat if you aren't making changes to the file. That way you review only the prettify changes and not the functional changes.
- jacquesm 11y ago
  This will condition reviewers to push 'merge' rather than to actually review the code because the assumption will be that these are safe changes.
- segmondy 11y ago
  I agree with you, beautifying and reformatting code can mess up the diff, and all of a sudden, someone ends up with their name pointing to code they didn't right. Beautifying, reformatting should only done alone.
- Myrmornis 11y ago
  Agreed, refactoring and functional changes should certainly be in separate commits, and ideally in separate pull requests. jacquesm can still have her/his rule to only change code involved in the course of functional changes; it's just that the refactoring and functional-change PRs will come in close succession and the refactoring PR exists only because the functional-change PR does also.

by junto 11y ago

jacquesm 11y ago

That's a great way to do it: fix-as-you-go. The same goes for adding unit tests where those are missing (in bulk, rather than as a set going with a recently touched piece of code), fixing up documentation issues and other global changes such as naming conventions and so on. Hitting all of the code base in one go with a thing like that is asking for trouble. Downvoters are cordially invited to state why they disagree with this.

junto 11y ago
I work on a team maintaining a large code base. We follow a similar rule concerning unit tests. Our testing over 5 years has slowly grown from 0% to give us ~75% coverage, by using the methodology you describe.

err4nt 11y ago

I'll offer another opinion on this - I an a frontend developer and so it's very common for me to have to deal with files that are written in PHP, HTML, JavaScript, and also sometimes including CSS embedded right in it. Roughly speaking, PHP and JavaScript have a similar syntax and can be formatted/styled in a similar way, but HTML it a totally different beast and has very different structure, as well as CSS which is a glorified list of rules. I don't refactor code for the sake of it, however, in an effort to read related files I often clean things up that I must read in order to edit another section. I don't discard my cleanups because I'm leaving it better than when I found it (and cleaning up a messy repository is tough, but a little in each commit can help). So I agree that you should style where you're working - but there are valid reasons to refactor or restyle code in other places too. Here's an example of a file I've been updating lately: - before: http://i.imgur.com/X37RFQD.png http://i.imgur.com/X37RFQD.png - after: http://i.imgur.com/9cjiJxn.png http://i.imgur.com/9cjiJxn.png

jacquesm 11y ago
There are exceptions to every rule, and yours is a valid exception to the rule that in general one should not commit non-functional change-sets. If something is particularly ugly then you could clean it up but that change should not be taken for granted not to have any adverse effects, there is really no such thing as an 'innocent' change. I've seen whitespace changes break code far too frequently to not be suspicious of any change at all.

dalore 11y ago

I believe that's the wrong way to do it. You can only prettify/reformat if you aren't making changes to the file. That way you review only the prettify changes and not the functional changes.

jacquesm 11y ago
This will condition reviewers to push 'merge' rather than to actually review the code because the assumption will be that these are safe changes.
segmondy 11y ago
I agree with you, beautifying and reformatting code can mess up the diff, and all of a sudden, someone ends up with their name pointing to code they didn't right. Beautifying, reformatting should only done alone.
Myrmornis 11y ago
Agreed, refactoring and functional changes should certainly be in separate commits, and ideally in separate pull requests. jacquesm can still have her/his rule to only change code involved in the course of functional changes; it's just that the refactoring and functional-change PRs will come in close succession and the refactoring PR exists only because the functional-change PR does also.