chrono
chrono copied to clipboard
chronotope
Fix rfc 2822 format when day is less than 10
Test : EDT.ymd(2015, 2, 3).and_hms_milli(23, 16, 9, 150) generates Tue,<space><space>3 Feb 2015 23:16:09 +0500 i.e. 2 spaces before day's value 3.
It should be Tue,<space>3 Feb 2015 23:16:09 +0500
Fixed it.
Sorry, this fell through the cracks, I'll think about it more deeply but a cursory glance looks like you're removing padding in cases where it is single-digits?
I believe that the current design double-spaces to keep multiple lines aligned no matter what day they fall on. Do you want to check if RFC 2822 explicitly states whether that is permissible?
@quodlibetor The current implementation keeps single space for days like 18 See test
The idea is to always keep only single space no matter whether is single-digit day or double-digit day.
The rfc recommends to fold the white spaces (replace multiple white spaces to single white space). From rfc :
Though folding white space is permitted throughout the date-time specification, it
is RECOMMENDED that a single space be used in each place that FWS
appears (whether it is required or optional); some older
implementations may not interpret other occurrences of folding white
space correctly.
day = ([FWS] 1*2DIGIT) / obs-day
The full grammar is :
date-time = [ day-of-week "," ] date FWS time [CFWS]
day-of-week = ([FWS] day-name) / obs-day-of-week
day-name = "Mon" / "Tue" / "Wed" / "Thu" /
"Fri" / "Sat" / "Sun"
date = day month year
year = 4*DIGIT / obs-year
month = (FWS month-name FWS) / obs-month
month-name = "Jan" / "Feb" / "Mar" / "Apr" /
"May" / "Jun" / "Jul" / "Aug" /
"Sep" / "Oct" / "Nov" / "Dec"
day = ([FWS] 1*2DIGIT) / obs-day
time = time-of-day FWS zone
time-of-day = hour ":" minute [ ":" second ]
hour = 2DIGIT / obs-hour
minute = 2DIGIT / obs-minute
second = 2DIGIT / obs-second
zone = (( "+" / "-" ) 4DIGIT) / obs-zone
Okay, I think you've convinced me. My inclination is that this is a breaking change and so it should wait for Chrono 0.5. It should get in there, so I've added it to that milestone. There's no real timeline for that release right now, sadly.
I would happily immediately take a PR to the chrono docs explaining the fact that the date is space-padded.
I would like to also say that I would also take a PR changing the alignment from space-padded to zero-padded, but the problem is that often, as I'm sure you've encountered, time-parsing code is sensitive to whitespace so any code that relies on this existing behavior might get busted if we eliminate double spaces. On the other hand, all code that can handle chrono-formatted RFC-2833 timestamps must already be capable of handling arbitrary whitespace, so maybe it's not that big of a deal, and I should just accept this PR after you've addressed my one comment. On the other other hand, accepting this PR will ruin alignment of any terminal apps that are depending on it for their display.
So I still think this needs to at least wait for 0.5, but I'm obviously a bit torn and am open to argument.
@quodlibetor I have addressed your one comment. I am not sure if this should go in 0.5 or before. I will leave that to your best judgement. Appreciate your patience for letting me do the fix.
Thanks @ashishnegi I am not sure that it's super important, but I am going to hold off on this until 0.5, unless other people object to that.
