Code review comment for ~racb/git-ubuntu:changelog-date-parsing

Revision history for this message
Bryce Harrington (bryce) wrote :

8d447a009bc64585603751c5ec766927cb3b565f
  - In code docs, param changelog_date_timestamp should be changelog_timestamp_string
  - strptime() throws a ValueError on parsing error. May be worth testing for in git_authorship()?

26a3f3b66b4ae429116f11f1c7c493fc4ff7fcc4
  - Looks good, a concise solution to a rather messy problem. The set of regex's provided seem to cover the various combinations, and looks easy to add more cases to.
  - In the assertion message, why is %r used ("Could not parse date %r" % changelog_timestamp_string,) ? It appears to behave properly, but I would have used %s so I'm curious.
  - Thanks for documenting the encountered cases for the invalid dates in the test case.

f5cdca0822e95d6954ffaf43db6a94819ea4bd3e
  - Setting LC_ALL also may be changing collation, which can change sorting behavior. I doubt that will be an issue at all for git-ubuntu, but if odd things start happening could look unsetting LC_ALL and instead setting LC_TIME and LC_CTYPE.
  - That aside, LGTM +1

review: Approve

« Back to merge proposal