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

Revision history for this message
Robie Basak (racb) wrote :

Thanks for the review! I've addressed your comments in a new branch, and
I'll merge that as soon as CI passes.

On Wed, Jul 22, 2020 at 04:57:12PM -0000, Bryce Harrington wrote:
> Review: Approve
>
> 8d447a009bc64585603751c5ec766927cb3b565f
> - In code docs, param changelog_date_timestamp should be changelog_timestamp_string

Fixed, thanks.

> - strptime() throws a ValueError on parsing error. May be worth testing for in git_authorship()?

I added a new test that verifies the known bad cases that should fail.

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

%r ensures to escape things that might be awkward for the terminal. As
we're reporting something wrong, it seems more likely than average that
this might happen here - for example with control characters or similar
that might not appear in a human-readable way in logs otherwise.

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

I agree there's a risk of disrupting things here. But ultimately we
probably want stable behaviour, so I think it's better to risk this
disruption now rather than later.

« Back to merge proposal