#9344 and policy for small bug reports

[Julien Phalip]

Hi,

I just wanted to draw your attention to what appears to be a bug in
Django: the 'tell()' proxy is missing from the Windows-specific
implementation of TemporaryFile. This causes Django to crash when
manipulating the uploaded file with PIL, for example. Ticket #9344
contains a patch to fix that.

Now, I know that this is sort of an edge case, and I also know that
there are more important and more urgent matters at this moment. But
I'd be keen to hear what is the official (or tacit) policy for that
kind of small bug reports. There probably are a few other tickets in
that situation (#9404 is another example). So, what is the best way to
go for people interested in having them checked in? Is it simply by
bringing them up on this mailing list from time to time? If so, then I
can try again after 1.1 lands.

Thanks a lot!

Regards,

Julien

http://code.djangoproject.com/ticket/9344
http://code.djangoproject.com/ticket/9404

[Ivan Sagalaev]

Julien Phalip wrote:
> There probably are a few other tickets in
> that situation (#9404 is another example).

And http://code.djangoproject.com/ticket/9591 is yet another.

[Russell Keith-Magee]

On Fri, Jan 23, 2009 at 2:38 PM, Julien Phalip <jph...@gmail.com> wrote:
>
> Now, I know that this is sort of an edge case, and I also know that
> there are more important and more urgent matters at this moment. But
> I'd be keen to hear what is the official (or tacit) policy for that
> kind of small bug reports. There probably are a few other tickets in
> that situation (#9404 is another example). So, what is the best way to
> go for people interested in having them checked in? Is it simply by
> bringing them up on this mailing list from time to time? If so, then I
> can try again after 1.1 lands.

Hi Julien,

Unfortunately, there's no simple answer to this question.

The short answer is that it is all about timing and opportunity.

A polite, well-timed message to the mailing list is certainly one way
to get attention. Keep an eye on the grand schedule. If you make noise
when the core devs are under the hammer trying to hit a feature
deadline or manage a planning phase, you're probably going to get
ignored. However, raising the ticket when the core devs are paying
attention to bugs - just before a bug fixing sprint, or in the leadup
to a beta release for example - is likely to get some traction.

Gentle IRC reminders can also work - again, strategically timed
(during a bug sprint would be a very good time, for example).

Another way to get traction is to pull related items together. When I
jump into the code to fix a bug in an area I haven't touched for a
while, it can take a few minutes to refresh my memory on exactly how
things work. If you collect minor bugs together into similarly themed
groups, you make an attractive target for us core devs (who are, after
all, exceedingly lazy and like easy jobs much more than hard jobs :-)

I wish I could give you a concrete answer (ask by email, written in
sanskrit, between 1 and 1:10 pm UTC on Tuesday afternoons :-) but like
all things open source, it isn't that simple.

Yours,
Russ Magee %-)

[Karen Tracey]

On Fri, Jan 23, 2009 at 12:38 AM, Julien Phalip <jph...@gmail.com> wrote:

Hi,

I just wanted to draw your attention to what appears to be a bug in
Django: the 'tell()' proxy is missing from the Windows-specific
implementation of TemporaryFile. This causes Django to crash when
manipulating the uploaded file with PIL, for example. Ticket #9344
contains a patch to fix that.

I probably looked at that ticket initially, at least briefly.  Here's a peek into what likely went on in my head:

- I should look at that...though I don't know the code involved...nor much about PIL...still, it's Windows-specific, I've got Windows boxes to test on, many (most...all?) other committers don't.
- Hmm....crash when manipulating uploaded file with PIL...do I know offhand how to recreate that...no....do I want to learn enough about PIL to dream up how to recreate it...not really.
- Maybe the patch has a test? Oh, there are 2 patches...I wonder why? They're identical...wha? Oh, the first had a spacing error.  But regardless, no test.
- Should there be a test?  Is this something that can't be tested? Is that why no test was provided?  Is it blindingly obvious to anyone who knows the first thing about PIL how to recreate this problem and that's why no specifics on how to recreate were included?  Dunno...this is too hard, let's find something easier to look at.

At this point I really should have noted in the ticket what stopped me from doing anything with it, but I didn't.  I'm bad that way, particularly when I get to a point of thinking that maybe it's my own lack of knowledge that's the problem.

So, what that ticket was lacking for me to look at it more closely was specific instructions as to how to recreate the problem so I could verify the fix.  Even better, a test integrated into the test suite, then it's clear to anyone looking later on what exact problem was fixed, and there is built-in protection against it breaking again.  If it's not feasible for some reason to add a test to the test suite then a note indicating why no test is possible would help.

Now, the fix may be trivial (and I'll agree it looks trivial), but I'm not going to check in anything without testing it.  Been there, done that, broke things, try real hard not to do it any more.  So I want to be able to see the problem myself before the fix and verify the problem is gone after the fix.

Now, I know that this is sort of an edge case, and I also know that
there are more important and more urgent matters at this moment. But
I'd be keen to hear what is the official (or tacit) policy for that
kind of small bug reports. There probably are a few other tickets in
that situation (#9404 is another example). So, what is the best way to
go for people interested in having them checked in? Is it simply by
bringing them up on this mailing list from time to time? If so, then I
can try again after 1.1 lands.

Best way to make sure "small" tickets do not get hung up on the way to checkin is to make them dead easy, even for someone who may not be intimately familiar with that area of the code, to understand the problem and verify the fix. Include tests integrated into the Django test suite that fail before the fix and pass afterward.  If integrated tests aren't possible, explain why the fix should be checked in even without tests, and include manual recreation instructions so the person who is considering the fix knows how to test it manually. 

Karen

[Julien Phalip]

On Jan 23, 5:14 pm, Russell Keith-Magee <freakboy3...@gmail.com>
wrote:

Thank you Russell for your clear explanation. In fact, I would copy/
paste your reply right into the Django documentation (http://
docs.djangoproject.com/en/dev/internals/contributing/#id1). Those tips
are very useful to know!

Julien

[Julien Phalip]

On Jan 23, 6:42 pm, Karen Tracey <kmtra...@gmail.com> wrote:

Thank you Karen for this detailed answer. Your reasoning regarding
this ticket does make a lot of sense. I totally agree with you that
tests are highly important and that this ticket is lacking useful
information for whoever is not familiar with that area of the code. If
I recall, the reason I hadn't written tests for the patch was because
of the way #7769 had been checked in shortly before 1.0's release. I
had thought that, like #7769, the patch was "trivial" enough for not
including tests. But again, as you said, the ticket was lacking
information and I should have at least put a link to #7769 in the
description. I've just done that. I'll think about writing tests but
it seems a bit overkill in this case for something which looks like a
small oversight.

Thank you,

Julien

http://code.djangoproject.com/ticket/7769

[Karen Tracey]

On Fri, Jan 23, 2009 at 3:14 AM, Julien Phalip <jph...@gmail.com> wrote:

Thank you Karen for this detailed answer. Your reasoning regarding
this ticket does make a lot of sense. I totally agree with you that
tests are highly important and that this ticket is lacking useful
information for whoever is not familiar with that area of the code. If
I recall, the reason I hadn't written tests for the patch was because
of the way #7769 had been checked in shortly before 1.0's release. I
had thought that, like #7769, the patch was "trivial" enough for not
including tests. But again, as you said, the ticket was lacking
information and I should have at least put a link to #7769 in the
description. I've just done that. I'll think about writing tests but
it seems a bit overkill in this case for something which looks like a
small oversight.

Pointing out where a similar change has been checked in without an added test might incline me more towards doing the same, if I'm feeling adventurous.  It might, on the other hand, push me towards thinking "hey, there's a bunch of stuff here that isn't being properly tested...we should correct that before proceeding to fiddle with any more of it."  Personally I tend towards the latter reaction...in this particular case I wonder what other methods may be missing here and is there some way to convince myself this ticket is the last of its kind we need to fix. 

But, if I decide to go ahead and just deal with this one, now, pointing out where a similar change has been checked in without a test doesn't get me past needing to manually verify for myself the existence of the problem and effectiveness of the fix.  I'm a terrible Python compiler and like to verify I've run a line of code before checking it in, if only to make sure it doesn't have any syntax errors.  For this ticket I still have no clue how to recreate this particular problem on my own or exercise the changed line of code.

So, you can wait for someone with more familiarity with this code to have time to look at it.  They may know immediately how to demonstrate the problem and verify the change fixes it, or they may feel it's patently obvious that the problem exists and the change fixes it with no syntax errors or anything, and just check it in.  Or, if you want to widen the circle of people who may conceivably move the ticket along, you can provide enough recreation detail in the ticket so that even someone who isn't familiar with this area of the code can recreate the problem and convince themselves they've exercised the changed line of code before checking it in.

Karen

[Graham King]

Very helpful information from Karen and Russell, thanks. I have
written it up for the Contributing section of the FAQ:

http://code.djangoproject.com/ticket/10110

[rajeesh]

I just want to register a similar issue. Had opened a ticket, #10057,
with patch on a trivial matter as http://code.djangoproject.com/ticket/10057.
Haven't found even any comments regarding its feasibility etc. Can't
figure out whether people are engaged for some milestone coming but
this seems to be a bad time. Lot of tickets are there remaining to be
un-reviewed. Feel like It'll be better If we can at least review the
problem as soon as possible. Assigning it to some one may take more
time. (Writing in Sanskrit and at odd times as per UTC seems not that
difficult for me. ;) ) Or else, the ticket owners may lose their
interest in the topic and later when someone takes charge, no one will
be there to give more feedback, when needed.. What can we do for that?

[Malcolm Tredinnick]

On Sun, 2009-02-08 at 09:22 -0800, rajeesh wrote:
> I just want to register a similar issue. Had opened a ticket, #10057,
> with patch on a trivial matter as http://code.djangoproject.com/ticket/10057.
> Haven't found even any comments regarding its feasibility etc. Can't
> figure out whether people are engaged for some milestone coming but
> this seems to be a bad time. Lot of tickets are there remaining to be
> un-reviewed. Feel like It'll be better If we can at least review the
> problem as soon as possible.

Anybody can review tickets (shouldn't review your own, for obvious
reasons). So if there are patches out there, start going through them.

The current timing is significant. Once we get a couple of things
knocked off and 1.1-alpha out the door, a few of us (and me, in
particular) will have more time to just dive in and start
closing/commiting/bouncing back tickets. It's not too hard to knock off
100 in a few days.

Regards,
Malcolm

[rajeesh]

> Anybody can review tickets (shouldn't review your own, for obvious
> reasons). So if there are patches out there, start going through them.
>
> The current timing is significant. Once we get a couple of things
> knocked off and 1.1-alpha out the door, a few of us (and me, in
> particular) will have more time to just dive in and start
> closing/commiting/bouncing back tickets. It's not too hard to knock off
> 100 in a few days.
>

That's really encouraging. Thanks Malcolm.