[Rpm-maint] [PATCH] Extend %changelog to support full timestamps
varekpaf at seznam.cz
Fri Oct 7 10:33:53 UTC 2016
Od: Pavlina Varekova <varekpaf at seznam.cz>Od: Panu Matilainen <pmatilai@
On 10/06/2016 05:36 PM, Florian Festi wrote:
"> On 10/06/2016 02:33 PM, Florian Weimer wrote:
>> On 10/06/2016 09:10 AM, Pavlina Varekova wrote:
>>> Extend %changelog to support full timestamps
>>> The newly accepted date format is
>>> Mon Jan 6 09:02:22 CEST 2016
>>> (like output of "date" command). Original format "Mon Jun 6 2016" is
>> I think it would be better to use ISO 8601 format (perhaps with space
>> instead of 'T' as the date/time separator). The week day information is
>> redundant and only causes consistency issues.
> I am really not a fan of the format but it is what's already implemented
> in rpm - especially on output with the :date and :day formats (which
> basically do date %a %b %d %Y and date %c). So this is the right way to
> go. I agree that the days of week are somehow redundant and an
> annoyance. But they ca be made optional later in another patch.
> Wrt the actual code: It's just a minor nit pick, but there are several
> lines duplicated in the two if / else blocks. Parsing the year and
> generating the final result can probably be done outside of the if else
> and those saving a few lines.
Yup, copy-paste code is bad even in small quantities, it'll bite back
Another minor nit:
* This is the rpm multi-line comment style, for better
* or worse.
Bonus points available by making the code more readable - cramming
multiple statements and if-actions etc into a single line is not a good
practise. The existing code doing something stupid is not a good reason
to duplicate it in new code :)
A whole other perspective on the parsing: is there an actual reason
strptime() cannot be used? It's not like I've tried, but I'd think you
could just call it with couple of different formats, or maybe even make
the "extended" format configurable via a macro.
- Panu -"
from my point it seems the format which I use could be there because the
majority is ok with it. Or am I wrong?
I will test whether strptime function can be used, - thanks for the tip. If
it can be used I will rewrite the patch using it. If not I will incorporate
the other Florian and Panu comments.
Thanks a lot for really fast feedback.
I fixed the "CEST 2016 in author field" bug in , thanks for spotting it.
I will add this to the pull request once I incorporate the strptime function
(it's usage has one side-effect: it is locale sensitive but I assume
changelog timestamp must be always in "C" locale not in e.g. fr_FR - it
would be quite complex to assume any locale in input).
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the Rpm-maint