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.
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: 5603751c5ec7669 27cb3b565f date_timestamp should be changelog_ timestamp_ string
> Review: Approve
>
> 8d447a009bc6458
> - In code docs, param changelog_
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.
> 26a3f3b66b4ae42 9116f11f1c7c493 fc4ff7fcc4 timestamp_ string, ) ? It appears to behave properly, but I would have used %s so I'm curious.
> - 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_
%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. 954ffaf43db6a94 819ea4bd3e
>
> f5cdca0822e95d6
> - 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.