Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#4937 enhancement closed fixed (fixed)

Fix for locale-dependant date formatting in imap and conch

Reported by: facundobatista Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/imap-conch-date-formatting-4937
(diff, github, buildbot, log)
Author: cyli, facundobatista Launchpad Bug:

Description

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).

Attachments (4)

locale-independent-formatting.patch (2.4 KB) - added by facundobatista 3 years ago.
locale-independent-formatting-with-tests.patch (5.4 KB) - added by facundobatista 3 years ago.
The same patch, but with locale-modifying tests
locale-independent-formatting-with-tests2.patch (6.1 KB) - added by facundobatista 3 years ago.
Auto-skipping tests and improved readability
locale-independent-formatting-with-tests3.patch (6.1 KB) - added by facundobatista 3 years ago.

Download all attachments as: .zip

Change History (33)

Changed 3 years ago by facundobatista

comment:1 Changed 3 years ago by glyph

  • Keywords review added

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

comment:2 Changed 3 years ago by cyli

  • Author changed from facundobatista to cyli, facundobatista
  • Branch set to branches/imap-conch-date-formatting-4937

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

comment:3 Changed 3 years ago by cyli

(In [31026]) Apply facundobatista's patch

refs #4937

comment:4 Changed 3 years ago by djfroofy

  • Keywords review removed
  • Owner set to facundobatista

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', ... }
  1. MONTH_NAMES is defined twice - should this not be in some common place?
  1. 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.

comment:5 Changed 3 years ago by 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
  1. 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
  1. 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

comment:6 Changed 3 years ago by 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.

Changed 3 years ago by facundobatista

The same patch, but with locale-modifying tests

comment:7 Changed 3 years ago by facundobatista

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

comment:8 Changed 3 years ago by facundobatista

  • Keywords review added

comment:9 Changed 3 years ago by djfroofy

  • Owner changed from facundobatista to djfroofy
  • Status changed from new to assigned

comment:10 Changed 3 years ago by djfroofy

  • Keywords review removed
  • Owner changed from djfroofy to facundobatista
  • Status changed from assigned to new

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.

comment:11 Changed 3 years ago by facundobatista

  • Keywords review added
  • Owner facundobatista deleted

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).

Changed 3 years ago by facundobatista

Auto-skipping tests and improved readability

comment:12 Changed 3 years ago by djfroofy

  • Owner set to djfroofy
  • Status changed from new to assigned

comment:13 Changed 3 years ago by djfroofy

  • Owner changed from djfroofy to habnabit
  • Status changed from assigned to new

+1

comment:14 Changed 3 years ago by djfroofy

  • Keywords review removed

comment:15 Changed 3 years ago by habnabit

  • Resolution set to fixed
  • Status changed from new to closed

(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.

comment:16 Changed 3 years ago by habnabit

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:17 Changed 3 years ago by 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.

comment:18 Changed 3 years ago by habnabit

  • Keywords review added
  • Owner habnabit deleted
  • Status changed from reopened to new

comment:19 Changed 3 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:20 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to habnabit
  • Status changed from assigned to new

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.

Changed 3 years ago by facundobatista

comment:21 Changed 3 years ago by facundobatista

  • Keywords review added
  • Owner habnabit deleted

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.

comment:22 Changed 3 years ago by facundobatista

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

comment:23 Changed 3 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

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.

comment:24 Changed 3 years ago by exarkun

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

refs #4937

comment:25 Changed 3 years ago by exarkun

  • Keywords review removed

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.

comment:26 Changed 3 years ago by exarkun

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

refs #4937

comment:27 Changed 3 years ago by exarkun

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

refs #4937

comment:28 Changed 3 years ago by exarkun

  • Resolution set to fixed
  • Status changed from assigned to closed

(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.

comment:29 Changed 2 years ago by exarkun

This was a duplicate of #3065.

Note: See TracTickets for help on using tickets.