[Rpm-maint] [PATCH] Extend %changelog to support full timestamps

Pavlina Varekova varekpaf at seznam.cz
Fri Oct 7 10:33:53 UTC 2016


Od: Pavlina Varekova <varekpaf at seznam.cz>Od: Panu Matilainen <pmatilai@
laiskiainen.org>
"
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
>>> still
>>> supported.
>>
>> 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 
one day.

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



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




Pavlina 
"



I fixed the "CEST 2016 in author field" bug in [1], 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).





Pavlina




[1] https://github.com/pavlinamv/rpm/commit/c95c6ff86df0e14bd85d0d746dd148e
2457a52b2

"
""
"
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20161007/77068c69/attachment.html>


More information about the Rpm-maint mailing list