Created attachment 132703 [details] [review]
Add RFC2822 format to soup_date_to_string() and use a leading zero for day of month

(In reply to comment #0)
> When passing a SOUP_DATE_RFC2822 SoupDateFormat to soup_date_to_string() it
> returns NULL. After having a short look at the sources, it seems there is just
> a case statement for the RFC2822 format missing to generate a proper date
> string.

oops. Though apparently I knew this at one point. If you look at tests/date.c, the RFC2822 test is #ifdeffed out. (You should fix that too, and make sure the test passes after that.)

> Also I think it might be better to use a leading 0 digit for the day of month
> in the RFC2822 date format. Though I couldn't find a clear statement about this
> in the RFC, date(1) seems to prefer a leading zero for the first 9 days of a
> month :).

What date(1) does doesn't matter though because it's not generating an RFC 2822 date. The fact that the grammar even allows a single-digit day strongly implies that leading 0s are not desired, and some of the examples elsewhere in the RFC use single-digit dates. Eg:

    Date: Tue, 1 Jul 2003 10:52:37 +0200

So we shouldn't use leading 0s.


Also, I'm pretty sure that SoupDate's offset has the opposite sign from RFC2822's offset, so you need to flip the '-' vs '+' test. (Making sure to still use '+' for the 0000 case though.) You should check that dates with offsets roundtrip between soup_date_new_from_string() and soup_date_to_string() properly.


Just out of curiosity, what are you working on that you need this to work for?

Created attachment 132712 [details] [review]
Version 2 of the patch

Ok, thanks for the feedback.

I don't have a strong opinion on the leading zeros in the day of month. When I was referring to date(1), I forgot to mention to use its -R switch to produce RFC2822 dates. Anyway, you are right that in the RFC examples no leading zeroes are used, so I removed that part from the patch.

I changed the code in parse_timezone() a little to parse timezone parts of a date without a separating colon.

And finally I changed the test program to test RFC 2822 dates and I changed the test date to contain a non-UTC timezone. Tests passed though I'm not 100% sure all is working fine now.

Btw, in the first patch there was a nasty bug which produced completely wrong timezone parts if the offset was negative, I fixed this by adding two abs() calls.

About my motivation on this:
I recently wrote a cookie manager for Midori, mainly just to view the cookies the browser uses. And when displaying a cookie, I used soup_date_to_string(cookie->expires, SOUP_DATE_RFC2822) to display the expiration time. Then I wondered why I always get NULL instead of a datetime string :). In the meantime, I'm using soup_date_to_time_t() and strftime() but adding this part to libsoup probably won't hurt.

Oh, forgot to mention: sorry if I broke the coding style. I wasn't sure at all about the indentation and stuff.

Created attachment 132715 [details] [review]
Version 3 of the patch

Oops, there was an unintended debug output included in the patch.

(In reply to comment #3)
> About my motivation on this:
> I recently wrote a cookie manager for Midori, mainly just to view the cookies
> the browser uses. And when displaying a cookie, I used
> soup_date_to_string(cookie->expires, SOUP_DATE_RFC2822) to display the
> expiration time.

Oh, you don't want to do that. :-) All of the formats soup_date_to_string() supports are non-localized, so you'll get English day and month names regardless of what language the user is using. So using strftime() is actually better. (Maybe there should be a soup_date_strftime()? Hm...)

But the patch is good anyway. I'll look at it more and commit it this weekend or something.

(In reply to comment #6)
> (In reply to comment #3)
> > About my motivation on this:
> > I recently wrote a cookie manager for Midori, mainly just to view the cookies
> > the browser uses. And when displaying a cookie, I used
> > soup_date_to_string(cookie->expires, SOUP_DATE_RFC2822) to display the
> > expiration time.
> 
> Oh, you don't want to do that. :-) All of the formats soup_date_to_string()
> supports are non-localized, so you'll get English day and month names

Yup, realised this as well :).


> regardless of what language the user is using. So using strftime() is actually
> better. (Maybe there should be a soup_date_strftime()? Hm...)

That would be nice though not strictly necessary. What I do now is:

expiration_time = soup_date_to_time_t(cookie->expires);
tm = localtime(&expiration_time);
strftime(date_fmt, sizeof(date_fmt), "%c", tm);

That's enough to get it working.


> But the patch is good anyway. I'll look at it more and commit it this weekend
> or something.

Cool.

pushed to git with some additional improvements and regression tests. thanks for the patch. this will eventually end up in 2.26.2