Discovered two bugs in _strptime.py thanks to Mikael
Sch?berg of AB Strakt; both were in
LocaleTime.__calc_date_time().  One was where if a
locale-specific format string represented the month
without a leading zero, it would not be caught.  The
other bug was when a locale just lacked some
information (in this case, Swedish's lack of an AM/PM
representation); IndexError was thrown because
string.replace() was being called with the empty string
as the old value.

I also took this opportunity to clean up some of the
code (namely TimeRE.__getitem__() along with
LocaleTime.__calc_date_time()).  Added some comments,
reformatted some code, etc.  All of this was brought on
thanks to the Python Cookbook's chapter 1 (good work
Alex and David!).

I have updated test_strptime.py to check for the second
of the mentioned bug explicitly.  I also commented the
code and added a fxn that creates a PyUnit test suite
with all of the tests.

Logged In: YES 
user_id=357491

Just when you thought you had something done, tim_one had to
go and normalize the whitespace in both _strptime.py and
test_strptime.py!  =)

So to save Tim the time and effort of having to normalize
the files again, I went ahead and applied them to the fixed
files.  I also reformatted test_strptime.py so that lines
wrapped around 80 characters (didn't realize Guido had added
it to the distro until today).

So make sure to use the files that specify whitespace
normalization in their descriptions.

Logged In: YES 
user_id=21627

Please don't post complete files. Instead, post context (-c)
or unified (-u) diffs. Ideally, produce them with "cvs
diff", as this will result in patches that record the CVS
version number they were for.

I think it would be good to get a comment from Mikael on
that patch.

Logged In: YES 
user_id=357491

Sorry, Martin.  I thought I remembered reading somewhere
that for Python files you can just post the whole thing.  I
will stop doing that.

As for Mikael and the patch, he says that it appears to be
working.  I gave it to him on Tuesday and he said it
appeared to be working; he has yet to say otherwise.  If you
prefer, I can have him post here to verify this.

Logged In: YES 
user_id=357491

Just as a follow-up, I got an email from Mikael on Mon.,
2002-08-12, letting me know that the patch seems to have
worked for the bug he discovered.

Logged In: YES 
user_id=357491

A bug issue was brought up in patch 474274 which was the
original _strptime patch.  I will address it and append it
to this patch.

Logged In: YES 
user_id=357491

So I have a patch for both _strptime.py and test_strptime.py
for Jason's bug, but I want to be able to run it through
PyUnit first.  For some strange reason, PyUnit will not
auto-execute my tests under my Python 2.3 install
(unittest.main()).  Hopefully the latest CVS will deal with
it.  If not, I might have a patch for PyUnit.  =)

Either way, I will have the files up, at the latest, at the
end of the month even if it means I have to execute the
tests at the interpreter.

Oh, and I will do a CVS diff (or diff -c) this time for the
patches instead of posting the whole files.

Logged In: YES 
user_id=357491

So I was right and now have submitted a patch for PyUnit on
its SF patch DB to fix the problem.  And now that I fixed it
I can say confidently that the changes pass the testing suite.

I have attached two contextual diffs below (couldn't get cvs
diff to work; still learning CVS); one diff is for
_strptime.py and the other is for test/test_strptime.py.

Logged In: YES 
user_id=33168

Brett, unfortunately Barry just checked in some changes to
strptime, so this is out of date.  (He fixed an am/pm
problem.)  

Could you generate new patches?  Also, could you separate
into separate patch files the bug fix/semantic changes from
docstring changes, whitespace, etc.  Typically, unrelated
changes are checked in separately.

To produce a diff from CVS, do:  cvs diff -C 5 > patch.diff
If you need CVS help, you can mail me directly.

Logged In: YES 
user_id=357491

Yeah, I noticed the email on Python-dev.  His patch 
actually overlaps mine since I did pretty much the same 
stylized changes in this patch (although I didn't have the 
AM/PM fix; wonder why that suddenly popped up?).  At 
least I don't have to deal with that now.

Since I am going to have to do a whole new diff I will try to 
separate the bugfix patches from the code style/doc string 
benign changes.

And thanks for the CVS help.  Hopefully I will be able to 
figure it out; if not, expect an email.  =)

Logged In: YES 
user_id=6380

Assigning to Barry so he can check this in.

Logged In: YES 
user_id=12800

Brett, I think the diff that's currently uploaded here is
not your latest one, which you mention in your 2002-08-30
15:26 follow up.  Can you see if you can fix the
test_date_time() failure that I emailed you privately today
also?

To recap for others reading this report, if the day of the
month < 10 (as it is today 3-Sep-2002), %d writes the
day-of-month field as "03" while %c writes it as " 3" in my
locale (C).  I don't know whether there are any guarantees
about what %c will return, so it's possible that the test is
just bogus.  I'm not sure what the intent of the test is, so
I've asked Brett in a private message for clarification or a
patch.

This is assigned to me, so I'll vette Brett's patch when he
uploads the newest version.

Logged In: YES 
user_id=357491

Yes, the current diffs do not have a fix for the problem you 
emailed me about.  They are also do not correspond to the 
newest CVS checkin that you did a few days back.  I will 
make separate patches for this newest bug (I think it is 
just a test_strptime.py bug), the case insensitivity fix, and 
my code cleanup/doc string cleanup.  I don't have an ETA 
on this, though.

Logged In: YES 
user_id=357491

I uploaded diffs against the CVS version of _strptime.py to
patch the two bugs reported.

missing_info_diff is a cvs diff -c that fixes the bug where
locale info is non-existent (the missing AM/PM stuff for
Swedish problem).  insensitive_fix_diff fixes the bug where
matching against different cases for locale info was not
deal with properly.

I created missing_info_diff first, then did
insensitive_fix_diff.  Hopefully I didn't mess anything up.

I still have to do my code cleanup diff and my doc diff. 
Those will be done by the end of the week (or at least I
plan on doing by then).

Oh, and can someone delete the old files?

Logged In: YES 
user_id=357491

I just re-uploaded the insensitive_fix_diff.  I realized
right after I posted my last comment that I didn't make sure
to catch some other places where insensitive case was
needed.  I have fixed those and they are now contained in
the diff for 2002-09-11.

Logged In: YES 
user_id=357491

OK, so I have uploaded a bunch of files (thanks, Neal, for
deleting the old files):

cleanup_diff -- cleans up __getitem__() in TimeRE.
comments_diff -- adds a single comment and adds an "a" in a
doc string.

c_test_diff -- fixes test_strptime.py's %c, %x, and %X tests
to use the magic date so as to avoid the issue that cropped
up of strftime using the funky ANSI space-leading day of the
month.
missing_info_test_diff -- Adds a test for the lacking locale
info bug.
case_test_diff -- Adds tests to make sure that
case-insensitivity is handled.

So, for _strptime.py, the order that the patches were
generated are:
missing_info_diff
insensitive_fix_diff
cleanup_diff
comments_diff

For test/test_strptime.py, the order of the patches were
generated are:
c_test_diff
missing_info_test_diff
case_test_diff

For all of these patches I just edited my local copy of the
CVS version of the file following my altered copy as a
guide.  When I did a set of edits, I generated a diff.  I
then continued to edit that same for the next diff.  Is that
the right way to do it?  Should I have deleted my edited
copy, get the CVS version, and edited that one?

Now I realize that test/test_strptime.py is not formatted
very well.  I am happy to fix my messy code and make all the
lines break at 80 characters, but I have a question about my
test patches before I do the reformatting.  For the new
patches I added the tests to the test methods I thought they
belonged in.  But I noticed after I generated the patches
that Barry put his 12-hour test as a completely separate
testing suite.  Should I redo my patches so that the tests
are separate?

Logged In: YES 
user_id=357491

I really need to stop playing with this code.  =)  I
generated another diff for _strptime.py that changes the
loop in TimeRE._seqToRE to use '|'.join().  Cleaner and also
alters the regex so that it doesn't explicitly use (?:)
since | binds so weakly.  The diff is join_diff.

Logged In: YES 
user_id=12800

I'm finally looking at these patches so that we can get them
into cvs now.  They didn't apply cleanly, but I'm not sure
if I'm applying them in the way that was intended.  Given
that all these changes are meant to be applied to a single
file (or two single files <wink>), maybe it's best to
generate a patch that contains all the changes, sync'd
against what's in 2.3a0 cvs right now?

Would that be hard to do?  If so, then let me know and I'll
try to work with what's there. 

Logged In: YES 
user_id=357491

So I uploaded two patches today (2002-09-23).  Both are just
cvs diff -c diffs for _strptime.py and test_strptime.py that
just take my fixed up copies and diffs with cvs.

Since Barry had problems (and everyone else who has ever had
to deal with patches from me has had issues), here is what I
did (this is all on OS X 10.2.1 and .cvsrc containing diff -c)::

cd path_to_CVS/python/dist/src/Lib
cvs diff _strptime.py > ~/another_path/big_strp_diff
cd test
cvs diff test_strptime.py > ~/another_path/big_test_diff

If there is some step there that I should not be doing (only
thing I can think of is doing it in the directory) please
let me know!  I have no clue why my patches never apply
cleanly and it is starting to really irk me.

And since this answers your questions, Barry, from your last
personal email I won't bother replying to it.

Logged In: YES 
user_id=12800

Good news Brett!  The patches applied cleanly.  I'll now
take a look at them.

Two minor comments about cvs diffing (these are my opinions
and maybe not shared by everyone).  First, it's easiest to
create the diff from the root of the Python source tree --
i.e. the parent of Lib.  That way, I can cd to the top
directory and do a "patch < your-diff" and won't have to
supply a file name.

Second, I personally prefer unified diffs to context diffs
since I (now) think they are easier to read.  Yes, I used to
be an anti-unified diff zealot, but I've become enlightened
<wink> and now generally prefer unifieds.  This is a
religious issue. :)

Will follow up after vetting the changes.

Logged In: YES 
user_id=12800

Looks good to me.  I had a couple of minor nits 

- long lines in _strptime.py, which I fixed but I'm not
going to touch test_strptime.py)

- a slightly optimization in TimeRE.__getitem__() where you
only need to create the constructors dict if a KeyError got
raised (no comment on the use of lambda :)

- test_time.py uses a %I format with no %p and this breaks
the ampm test in strptime(), because you try to
None.lower().  I fixed this by using found_dict.get('p',
'').lower() and equating a '' return value to AM.  Test
passes so I assume this is okay <wink>.

I'm prepared to commit these changes and close this issue,
if Brett agrees.

Logged In: YES 
user_id=357491

So to respond to your three points::

- Sorry about that!  I thought I had caught all of the long
lines and broke them.

- OK.  I thought the dict would be created in the opcode
once and thus not be a big issue.  My mind must have just
slipped into compiler mode.  =)  And as for the lambdas, I
used them just because they work.  I know I could have used
*args, but it looked ugly in the dict and it required
another dict call.  I figured lambda would be faster.  If I
am wrong, go ahead and change it if you care to, Barry.

- Damn.  Overlooked that.  Thanks for catching it.  And as
for the correctness of the test, if the test passes then it
is correct.  =)  Since they didn't specify AM/PM for a
12-hour time they get what they get for the hour.  =)

So I say go ahead and apply the patches if you are happy
with them, Barry.

Now, about test_strptime.py being kind of a mess in terms of
long lines.  I am willing to clean it up.  If I do, should I
start another SF patch?  Or is it not worth the bother?

Logged In: YES 
user_id=12800

It's probably not worth the bother if you have better things
to do.  If you do choose to clean it up, then you can either
just check it in (I forget if you have cvs write access), or
upload it as a new patch and assign it to me.  I'm going to
commit the changes and close this issue.