On Jan 23, 6:42 pm, Karen Tracey <kmtra...@gmail.com> wrote: > On Fri, Jan 23, 2009 at 12:38 AM, Julien Phalip <jpha...@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
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 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~----------~----~----~----~------~----~------~--~---
