By dman on

Following the delayed branch of the DBA module as discussed here I'd like to pose a question to Module/CVS maintainers...

Do you really expect contributors to submit functionality changes without any code style repairs, or vice-versa, bother with code style repairs if they are not doing any bugfix or additional functionality?

<X-POST>...
I sympathise with both sides of the story here. :-/

I find it really hard to split my head in three when trying to help fix up a module. In the course of porting, it's hard to resist repairing code style issues where I see them, and usually the only reason I'd be opening up the code in the first place is to actually bugfix or add a feature. This inevitably leads to a three-in-one patch :(
However, even if I did get enough encouragement to go back and try to branch those bits into 3 separate patches ... how does one go about that?

Should random style/text/comment fixes come first? (effectively repairing a release you are not even intending to, or even able to, run - which may be critically broken in the first place - boring)
Or should the port come first, ignoring style changes? (Which I'd find really hard as I often have to unfold the old code to the point I can understand what it used to do. Running an IDE code formatter over the thing is usually my first step.)

Should the second patch be built assuming the first is applied? (I'm certainly not able to go back and create several with-and-without branches just for my own 'one' change)
Therefore is there any point in even submitting the second useful patch to the issue queue as it will not apply cleanly to anything until the first patch is committed ... which may take a week or two.

Thoughts? How best to help everyone work together sanely? Best practices for patch submissions?

Comments

dww’s picture

dww
we/he/they
commented

(Btw, I'm not calling dman stupid -- this is a great question, and the outcome should be doc'ed the best practices area...) ;)

Obviously the best thing is when the maintainer is ultra careful about the coding conventions in the first place and it's not an issue. However, if you find yourself trying to port something and find a sea of ugly, non-standard-compliant code, here are your options:

  1. Ignore the code style problems in the code you see around you, and limit yourself to changes to the core API. Obviously, your new code should follow the standards, but don't touch anything else. If the module is so horribly formatted that you can't even read it before you begin porting, see option #2...
  2. Do a separate, complete code-style cleanup patch as another issue. Mark it as a critical task. So long as it's *JUST* code style, it should be easy to review, test, and commit quickly. Then, work on the porting.

Anything else will make problems for the maintainers. Sadly, if they didn't care enough about code style to write it correctly in the first place, they might not understand the importance or benefits of doing it like this. Here's some evidence to support my claim. If you change a ton of code formatting and style issues in the same patch (and same CVS commit) as a port to the new API, this is what will befall you:

A) During the porting itself, it makes it much harder to review the patch for the API changes, since the patch is full of stuff that has nothing to do with the porting. Reviewers have a much harder time seeing if the patch is valid, that it's not introducing bugs or security problems, etc. This will delay getting the patch in, and make everyone stressed out and unhappy.

B) You basically ensure that all future patches for bug fixes will have to be re-written for each branch you're still maintaining. If you're an irresponsible maintainer, and only ever worry about 1 branch at a time, and forget about bug fixes and security releases for people using older versions of core, this might not bother you. But, if you're responsible, this is a big deal. It means much more work in the long run to maintain security and bug fix releases on the older branches. It also means that if anyone provides a patch for one of your bugs, they will most likely only patch the branch they happen to care about. So, you're making more work for yourself, and more work for the people trying to help you.

C) It makes it much more confusing to see what changes to the API actually affected your module. If it turns out something went wrong with the porting, it's takes *much* longer to see what functional changes happened between the 2 branches, since the cvs diff will be full of noise (code style/formatting changes), drowning out the signal (API changes). Again, more work for you...

I could probably keep going, but hopefully that's enough to convince people.

Cheers,
-Derek

___________________
3281d Consulting

dman’s picture

dman commented

You've identified the drawbacks and problems indeed. I totally agree with the way we get into these situations.

I'm looking for a way to modify my own workflow that doesn't put the cart before the horse.
Usually, when opening module code, I'm not there to do maintainence, I'm there to see if I can make it work a little bit more like I want.

This involves:

  1. Locating the existing code surrounding my desired functionality
  2. Popping in a bit of debug to see what's happening, - possibly enhancing the comments while I'm there
  3. Trying a hard-coded tweak to see if I can change it OK
  4. Finding the appropriate place to add a configuration setting, as I'm careful to leave the module behaving as expected and adding my option as an extension.
  5. In doing so, I often format the surrounding code to make it consistant and 'readable' to me, most often unfolding $form arrays onto separate lines for visibility.
    Having a mix of styles in one function is painful to me.
  6. Usually I add a sentence or @param or @return to the existing function(s) doc-block, for my own satisfaction/understanding, as I like docblocks, although module owners often (understandably) skip over them a bit.
  7. Test and remove debug (possibly messing up old whitespace)
  8. Run a code check, nowadays using coder.module. Making any trivial fixes I find in order to identify any real issues. I've learnt that submitting non-conformant patches generally gets your contributions dismissed for pedantic reasons rather than practical ones :( and that's discouraging.
    I thought that would be helpful, and, on the face of it, trivial :-/

Roll a patch ... which ends up having all three types of changes in it :(

I see that the last step can probably be left out, but still...
Fixing comments and some layout is something I have to do before adding my 'improvements', yet is seldom something patch-worthy to send in as a contribution on its own :-/

Perhaps there's a better way of rolling patches I don't know about, whereby I can submit only the relevant bits. I've sometimes looked at hand-editing the patch file, but am worried about line number changes if I just chop things out...

.dan.
How to troubleshoot Drupal | http://www.coders.co.nz/

.dan. is the New Zealand Drupal Developer working on Government Web Standards