Quoting nids in comments
What do people think about mentioning the issue nid in the comment for some of the more trickier/obscure fixes? I'm not sure if this already being done - I haven't come across any instances of this. But adding the nid will save a lot of time and effort for everybody IMO.. Just a thought. Cheers, -K
On 3/16/06, Karthik <narakasura@gmail.com> wrote:
What do people think about mentioning the issue nid in the comment for some of the more trickier/obscure fixes? I'm not sure if this already being done - I haven't come across any instances of this. But adding the nid will save a lot of time and effort for everybody IMO..
http://cvs.drupal.org/viewcvs/drupal/drupal/modules/node.module That is already standard practice. nid and drupalID of the patch provider. Greg
On 3/16/06, Karthik <narakasura@gmail.com> wrote:
That is already standard practice. nid and drupalID of the patch provider.
I mean in the code itself.. not the commits:
// Equate $a to $b (#43912) $a = $b;
Continuing to use node.module as an example, Lines in File: ~2400 Lines with content: ~1590 NCLC: ~1235 Comments: ~350 Bugs listed in CVS commit messages: ~260 So you are basically asking for a doubling of comments. Does that really help readability? What about parts of code that have many bugs filed against them? //Equate $a to $b (#1234) //Clone b along the way (#1235) //We need drupal_clone (#1236) $a = drupal_clone($b) Is that an improvement over having that knowledge in the commit messages? This strikes me as a bad idea so I want to lay out the implications - but it clearly isn't my decision. Regards, Greg
So you are basically asking for a doubling of comments. Does that really help readability?
Quoting my original e-mail : "What do people think about mentioning the issue nid in the comment for *some* of the more *trickier/obscure fixes*?" This is just an attempt to provide a starting point for the person trying to understand the code. -K
Comments should document how the code works. Historical clarification should be left for CVS messages IMO. --Jeff
-----Original Message----- From: Karthik [mailto:narakasura@gmail.com] Sent: Thursday, March 16, 2006 9:17 AM To: development@drupal.org Subject: Re: [development] Quoting nids in comments
So you are basically asking for a doubling of comments. Does that really help readability?
Quoting my original e-mail : "What do people think about mentioning the issue nid in the comment for *some* of the more *trickier/obscure fixes*?"
This is just an attempt to provide a starting point for the person trying to understand the code.
Karthik wrote:
What do people think about mentioning the issue nid in the comment for some of the more trickier/obscure fixes? I'm not sure if this already being done - I haven't come across any instances of this. But adding the nid will save a lot of time and effort for everybody IMO..
Just a thought. Cheers, -K
Just checking... are you aware of the annotate feature @ http://cvs.drupal.org/viewcvs/drupal/ ? I ask because I wasn't until very recently, and it is the freaking bomb. It seems like it is perfect for situations like you describe. Let's take a look at form.inc, for example: http://cvs.drupal.org/viewcvs/drupal/drupal/includes/form.inc?view=markup Inside here we see the following line: if ($form_values['form_token'] != md5(session_id() . $form['#token'] . variable_get('drupal_private_key', ''))) { Form tokens? Gee, I wonder what that was all about. Click the "annotate" link near the top of the file: http://cvs.drupal.org/viewcvs/drupal/drupal/includes/form.inc?annotate=1.79 Now you get, line by line, the file version numbers where that line first appeared, along with who committed it. So we see that line originally appeared in version 1.59 of the file. If you click "1.59", it will show you a diff between that version and the one before it. Click "Revision Log" and you're back to a full view of all file revisions. Now you can Ctrl+F for "revision 1.59" and you'll see: - Patch by chx: made form tokens work. Haha, ok so I picked a bad example. :P~ But the revision before that has the message: - Patch #46227 by yogadex: fixed problem with form validation. And sure enough, @ http://drupal.org/node/46227 you can see the discussion about the original patch and the follow-up that's in HEAD now. If you already knew about that feature and you don't find that good enough then my apologies but it's been a life-saver to me, so figured I would share it with others who might not have come across it. -Angie
If you already knew about that feature and you don't find that good enough then my apologies but it's been a life-saver to me, so figured I would share it with others who might not have come across it.
No apologies necessary :) I had niwse! For the record, the commandline options are just 'cvs annotate'/'svn annotate' and I'm sure there's a 'bzr annotate' as well :) Cheers, -K
participants (5)
-
Angie Byron -
Greg Knaddison -
Jeff Eaton -
Karoly Negyesi -
Karthik