Fix rfc 2822 format when day is less than 10

[ashishnegi]

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.

[ashishnegi]

I tried : let fmt_str = if (d.day() < 10) { "{}, {:1} {} {:04} {:02}:{:02}:{:02} " } else { "{}, {:2} {} {:04} {:02}:{:02}:{:02} " };
but then I was stuck at error error: format argument must be a string literal. when i tried to assign format string to a variable.
My rust is not strong.
Let me know the right way to fix it, i will do that.

[ashishnegi]

Also, Is Tue, 03 Feb 2015 23:16:09 +0500 be better ?

[quodlibetor]

Since this number is guaranteed to be between 1 and 31 inclusive, it should be fine to just remove the the width specifier -- just use {}. I think that's what should be done here.

If you ever need to deal with something like this again, though, you can use the $ width specifier to supply the width as a separate argument. In this case it would be something like:

let wwwwidth = if d.day() < 10 { 1 } else { 2 };
try!(write!(w, "{}, {:$} {} {:04} {:02}:{:02}:{:02} ",
    SHORT_WEEKDAYS[d.weekday().num_days_from_monday() as usize],
    d.day(), wwwwidth, SHORT_MONTHS[d.month0() as usize], d.year(),
    t.hour(), t.minute(), sec));

Again, I don't think we should do that here, but it's kind of nifty.

[ashishnegi]

Done.

[ashishnegi]

Any suggestions ?

[quodlibetor]

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?

[ashishnegi]

@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

[quodlibetor]

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.

[ashishnegi]

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

[quodlibetor]

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.

[ashishnegi]

@quodlibetor Are we going to merge this ?

[ashishnegi]

Ping.

[pickfire]

Seems dead.