Fwd: [drupal:unconed] /modules/taxonomy taxonomy.module
Issue nid? ---------- Forwarded message ---------- From: drupal-cvs@drupal.org <drupal-cvs@drupal.org> Date: 11-Apr-2007 14:47 Subject: [drupal:unconed] /modules/taxonomy taxonomy.module To: drupal-cvs@drupal.org User: unconed Branch: HEAD Date: Wed, 11 Apr 2007 09:17:53 +0000 Modified files: /modules/taxonomy taxonomy.module Log message: - Fix bad code in taxonomy.module (variable naming) Links: http://cvs.drupal.org/diff.php?path=drupal/modules/taxonomy/taxonomy.module&...
Karthik wrote:
Issue nid?
---------- Forwarded message ---------- From: drupal-cvs@drupal.org <drupal-cvs@drupal.org> Date: 11-Apr-2007 14:47 Subject: [drupal:unconed] /modules/taxonomy taxonomy.module To: drupal-cvs@drupal.org
User: unconed Branch: HEAD Date: Wed, 11 Apr 2007 09:17:53 +0000
Modified files: /modules/taxonomy taxonomy.module
Log message: - Fix bad code in taxonomy.module (variable naming)
Links: http://cvs.drupal.org/diff.php?path=drupal/modules/taxonomy/taxonomy.module&...
it is obvious from the diff that the author used an incorrect variable name. IMO, there is no need to review such a trivial patch. Nor is it really necessary to trouble 1000+ subscribers with such a question. A private email would do. i really want to keep a high signal/noise ratio on this list, thus my pickiness. i hope noone takes it personally.
it is obvious from the diff that the author used an incorrect variable name. IMO, there is no need to review such a trivial patch. Nor is it really necessary to trouble 1000+ subscribers with such a question. A private email would do.
a) When a process has been established, it has to be followed and followed diligently. Straightforward patches might be readily apparent now, but 2 years down the line, might not. A cvs annotate will not show up anything. b) I believe that this is the third core patch in the last few days that is missing an nid. c) I have a patch related to the same block of code [1], and if an issue exists for this patch, it might have information related to my code.. or might not .. or my issue might be a duplicate. I don't know. d) The last time I tried bringing an issue to Steven's attention via his contact form, I did not receive a reply. e) This is on topic. f) Transparency ++. g) I still don't know the nid. -K [1] http://drupal.org/node/134524
On 11.04.2007, at 16:10, Karthik wrote:
it is obvious from the diff that the author used an incorrect variable name. IMO, there is no need to review such a trivial patch. Nor is it really necessary to trouble 1000+ subscribers with such a question. A private email would do.
g) I still don't know the nid.
http://drupal.org/node/135500. Now only Steven needs to add the "Committed to HEAD" to the issue and close it. Konstantin Käfer – http://kkaefer.com/
http://drupal.org/node/135500. Now only Steven needs to add the "Committed to HEAD" to the issue and close it.
Thank you kindly. -K
This was a *community reviewed* patch that got committed only a week before, and which had some bad last minute changes in it that no-one noticed when it was committed. The review process exists not to satisfy your desire for bureaucracy, but to make sure the community can provide valuable feedback on changes to Drupal. But like all other process involving people, it is flawed, and sometimes mistakes get committed. In the case of really trivial bugfixes (like this) or typo corrections, there is absolutely no reason to object to the change, and thus no reason to waste everyone's time with an issue and review. The issue that Konstantin created says just as much as the commit message. As the original author of the patch that got the bad code in, fixing it directly seemed completely reasonable and acceptable *for this particular case*. It was only a variable renaming after all. For all the other changes I propose to Drupal, I create patches like everyone else and rarely commit my own patches (and usually only after Dries gives me a personal thumbs up). Steven Wittens
On 14/04/07, Steven Wittens <steven@acko.net> wrote:
This was a *community reviewed* patch that got committed only a week before, and which had some bad last minute changes in it that no-one noticed when it was committed. The review process exists not to satisfy your desire for bureaucracy, but to make sure the community
My desire for bureaucracy? hah! Please don't confuse bureaucracy with process or work-flow. The results are plainly evident in the issue in question, where Dries had no idea that the commit had already been made and wasted X minutes of his life, and more importantly, review and commit time. The question of patch triviality is of no relevance here, considering that an issue exists. As stated earlier, this is not the first commit that this has happened to recently. Off hand, the last commit to tableheader.js also lacked an nid or credit, and that was not a readily apparent patch.
In the case of really trivial bugfixes (like this) or typo corrections, there is absolutely no reason to object to the change, and thus no reason to waste everyone's time with an issue and review.
If it is trivial, then, IMO, there is no need for a review. However, please create an issue and mark it fixed directly and include the nid in your commit message. What if Drumm or Killes want to back-port a trivial patch? Or if what you thought was trivial ... wasn't? As you've so eloquently put, processes involving people are inherently flawed. You are currently leaving no avenue for any community feedback altogether. Moreover, this is CVS. If I check my CVS log six months from now, I will have no idea whether you committed one file or ten thousand. I will have to chase up the commit via the commit message to find out. Instead, please spend the 15-30 seconds required to create an issue and mark it fixed. And on a related note, I would also like to request that you please stick to the convention adopted by all other committers and include attribution in your commit messages. I am well aware of your views on this matter. However, whether it is right or wrong is besides the point and is something that you guys can iron out between yourselves and / or with the community. In the meantime, it will be appreciated if all of you are consistent. I would also like to point out that your preference to avoid attribution in commit messages is decidedly at odds with your theme .info patch wherein you were / are pushing for an attribution field on the theme page. Please also understand that I'm not having a go at you personally; I understand that a committer's job is difficult and generally thankless. All I'm trying to establish here is consistent behaviour that will save time and energy for all parties concerned by generally reducing confusion all around. Thanks, -K
On 4/17/07, Karthik <narakasura@gmail.com> wrote: <lots good stuff> I agree with Karthik on most of his points. Every time I see my username in a commit message it inspires me to write more patches. Every time it's omitted, it makes me feel a little sad. I've ranted on this before so I'll try to be short today ;) I know that the job of core committers is already hard. To try to address that and promote the use of more consistent commit messages I propose this change to the bluebeach theme: http://drupal.org/node/98165 The idea is to provide a little line at the bottom of every issue that contains the template of the commit message. This will reduce the time that commiters spend typing up the message and will help promote the use of the template by contrib maintainers (few of whom are aware of the standard). Reviews and criticisms welcome. And if it does get committed I'd appreciate getting credit ;) Regards, Greg
On Apr 17, 2007, at 8:08 AM, Greg Knaddison - GVS wrote:
I agree with Karthik on most of his points.
for what it's worth, me too.
I know that the job of core committers is already hard.
agreed. and often thankless... i just wanted to reiterate my appreciation for all of the amazing "gatekeepers" we've got on this project.
The idea is to provide a little line at the bottom of every issue that contains the template of the commit message.
i've got a few concerns i posted in the issue, but overall, i think it's worth giving this a try...
Reviews and criticisms welcome. And if it does get committed I'd appreciate getting credit ;)
hehe, except only about 6 people on earth have access to the CVS repository where bluebeach lives, so it's not going to do much good. you'll probably never see the commit message yourself, to get that warm fuzzy feeling of being appreciated. ;) but if i commit it, i promise i'll include your name in there, just in case. ;) cheers, -derek
The results are plainly evident in the issue in question, where Dries had no idea that the commit had already been made and wasted X minutes of his life, and more importantly, review and commit time. The question of patch triviality is of no relevance here, considering that an issue exists.
The issue was created *after the commit* and did not reflect the actual state of the patch (fixed). This is what caused the confusion, not the fact that a trivial bugfix was directly committed. Steven Wittens
The issue was created *after the commit* and did not reflect the actual state of the patch (fixed). This is what caused the confusion, not the fact that a trivial bugfix was directly committed.
Thank you for the clarification. -K
On 28.04.2007, at 12:57, Karthik wrote:
The issue was created *after the commit* and did not reflect the actual state of the patch (fixed). This is what caused the confusion, not the fact that a trivial bugfix was directly committed.
Thank you for the clarification.
I created the issue when I read your initial message and though that it was clear that this was irony (cf. the "review" by fake_user_account and the reference to the CVS commit in the initial issue description). I am sorry for the confusing I may have created. Konstantin Käfer – http://kkaefer.com/
participants (6)
-
Derek Wright -
Greg Knaddison - GVS -
Karthik -
Konstantin Käfer -
Moshe Weitzman -
Steven Wittens