Fix for locale-dependant date formatting in imap and conch

[facundobatista]

These modules had a date formatting using time.strftime() with the '%b' formatter, that is locale dependent.

So, if you run any of these in a non-English machine, you'll break a little the protocols.

As discussed in the mail list, these dates should be formatted using always the English month abbreviation.

This patch fixes that.

Also, I took the liberty to improve a little the code in conch/ls.py around the date formatting modifications (adding strings is really expensive).

[glyph]

Looks like this ought to be in review, since there's a patch.

[cyli]

(In [31025]) Branching to 'imap-conch-date-formatting-4937'

[cyli]

(In [31026]) Apply facundobatista's patch

refs #4937

[djfroofy]

There is no unit test which demonstrates the bug. I think it's sufficient to write one so the test would fail on a builder set with a different locale - or maybe do something to change locale setUp() and set back in tearDown() (or with addCleanup) to make the test fail anywhere - though that might be a bad idea.

Some other notes:

1. I think it helps readability to define MONTH_NAMES constants less trickily. I.e. just: MONTH_NAMES = { 1: 'Jan', 2: 'Feb', ... }

2. MONTH_NAMES is defined twice - should this not be in some common place?

3. Some parts of the patch seem unrelated to this ticket - namely in lsLine() you are replacing some string concat with join on list at the end; I think the latter is better but maybe should be a separate ticket/patch.

[facundobatista]

Hi! Thanks for the review!

There are unittests that exercise that code... the only wait to make the code fail is to change the locale, which we can do in a test (but yourself said it may not be a good idea)

Regarding your other points...

1. usage of zip and range is not "tricky", IMO

2. I put it in different places because I couldn't find a common one (and was simple enough), if you know where I could put it, I'd be glad to move it

3. I said above, as I was touching that code, just replaced a zillion '+' for a .join(); the change is not that important, so I minimized overhead including it in the same patch

[djfroofy]

Do the unit tests which exercise the code result in test failure(s)?

I'm also of the opinion also that there should be a specific test for a bug rather than relying on side-effects of other tests.

[facundobatista]

The same patch, but with locale-modifying tests

[facundobatista]

There I attached a new patch, this time including tests that actually exercises the same code, but changing the locale first.

[djfroofy]

First, the new tests ERROR on my machine because of failure setting locale:

locale.Error: unsupported locale setting

I don't know of the best way to get around this. Maybe pick a more common locale which is less likely to cause failure. Then I think the test needs to be skipped instead of causing error if setting the more common locale still causes an error.

Second, for readability, in lsLine() on line 51 there is a mixture or vars and inline calls to get values (str(s.st_nlink).rjust(5) vs. un.ljust(9)). Maybe just make consistent by getting rid of the variables and just putting everything inline when building the list.

[facundobatista]

djfroofy: As we discussed, configured the test to being skipped if the locale is not installed (even if not running, is a good documentation to have).

Regarding the readability in lsLine(), I improved it a lot (IMHO, hope you like it).

[facundobatista]

Auto-skipping tests and improved readability

[djfroofy]

+1

[habnabit]

(In [31080]) Merge imap-conch-date-formatting-4937: fixing locale-dependent date formatting

Authors: cyli, facundobatista Reviewer: djfroofy Fixes: #4937

This removes a dependency on locale-specific formatting directives used in t.conch and t.m.imap4 and instead only uses English month abbreviations.

[habnabit]

 build results <- committed a fix to work on 2.4; buildbot is still chugging along, but it doesn't break on 2.4 anymore.

[exarkun]

Thanks! Some simple things:

1. testFetchInternalDateLocaleIndependent should be test_fetchInte...
2. Don't say should be in a test doc string! Say is!
3. All of the current_locale variables should be currentLocale (3 or so of them in 2 different files)
4. The indentation of MONTH_NAMES is kind of messed up in imap4.py. Also please make it private. :)
5. assertEquals is preferred over assertEqual (seen in test_cftp.py)
6. Likewise for MONTH_NAMES in ls.py
7. lsLine needs a docstring, too.

[facundobatista]

Attached "locale-independent-formatting-with-tests3.patch"

Addressed:

1. Fixed.
2. Fixed.
3. Fixed.
4. Made it private. Didn't understand about indentation issue.
5. I prefer to user assertEqual over assertEquals, as the later is not documented anywhere (not in twisted docs, neither in Python docs), so it can go away at any time.
6. Same as (4)
7. Fixed.

[facundobatista]

One further comment regarding point 5: assertEquals is *deprecated* in Python 3 (another reason to stop using it).

[exarkun]

One further comment regarding point 5: assertEquals is *deprecated* in Python 3 (another reason to stop using it).

Yes! We are exceptionally bad at predicting future behavior of python-dev. Some short while before python-dev decided to deprecate assertEquals, Twisted standardized on it. Trial was already providing its own implementation, so we don't see any deprecation warnings from the stdlib version either.

[exarkun]

(In [31119]) Apply locale-independent-formatting-with-tests3.patch

refs #4937

[exarkun]

Cool! Thanks again for the patch. I'm going to make a few minor tweaks:

1. The _MONTH_NAMES indentation I mentioned was just aligning the 2nd line of args with the ( on the 1st line.
2. assertEqual changed to assertEquals (for now :) as mentioned in my earlier comment and as discussed IRL
3. test_fetch... instead of test_Fetch...

... test it on buildbot, and then merge.

[exarkun]

(In [31123]) Minor tweaks to whitespace and naming, plus news fragments

refs #4937

[exarkun]

(In [31126]) Use double try instead of try/except/finally

refs #4937

[exarkun]

(In [31137]) Merge imap-conch-date-formatting-4937

Author: facundobatista, habnabit Reviewer: djfroofy, exarkun Fixes: #4937

This removes a dependency on locale-specific formatting directives used in t.conch and t.m.imap4 and instead only uses English month abbreviations. This re-merge fixes a Python 2.4 incompatibility.

[exarkun]

This was a duplicate of #3065.