[phobos] std.gregorian [was phobos commit, revision 1919]

[Yao G.]

On Wed, 25 Aug 2010 11:34:10 -0500, Lars Tandle Kyllingstad  
<lars at kyllingen.net> wrote:

> I've had the chance to look at your date lib now, and (without being
> anywhere near an expert on the matter) I think it seems rather good.

Thanks!

> 1. I'm not sure I agree that DayOfWeek should be a named enum.
> Personally, I'd prefer if the weekdays were enumerated like the months,
>
>   alias int DayOfWeek;
>   enum : DayOfWeek { Monday, Tuesday, ... }
>
> That way you can get rid of the DOW alias.  In general, I'm not fond of
> aliases that are just abbreviations, so I suggest removing WeekNo as
> well. ;)

TBH, I dislike anonymous enums. They tend to polute the scope where they  
are imported, and sometimes the value name is too ambiguous without the  
enum name prepended. But in this case, I think the week days are  
self-describing enough. In fact the enum with the month names also started  
as named enum, but after writing MonthName.September for the tenth time, I  
got tired and changed it. Yes, this is a good idea to make this change.

> 2. The month enum should have the Month type, I guess?
>
>   enum : Month { January, February, ... }

Well, it kinda get complicated. Originally, the Month alias was going to  
be a full fledged structure, but then the things started to get unwieldy  
because as the month can also be represented with a number (or an enum), I  
would had to write a lot of operator overloads (for mathematical  
operations and comparison with ints). So at the end I settled with an  
integer. But if there's the need to revert back to the Month struct, then  
the enum wouldn't make sense (an enum of structs?). Short version: Month  
can change of type so that's why the enum shouldn't be of the  
aforementioned type.

>
> 3. I found it rather strange that the doc comment says that
> DatePeriod.last() returns the day *after* the period, while
> DatePeriod.end() is the last day of the period.  I checked
> boost::date_time, and indeed it seems you've switched the two -- at
> least in the documentation.

Yes. Those are some silly mistakes I did and I'll correct them pronto.

> 4. The doc comments need a bit of cleaning up.  Examples:
>   - There should be some ///ditto comments for the Month,
>     Day, and DayOfYear types.

Yes. I'll do that.

>   - The docs for DateRange.this() refer to WeekRange.

Yes, that's one of the perils of copy/paste :( I copied the definition of  
WeekRange and refactored it to GenericRange but I forgot to update the  
docs.

>   - The docs for Date.toIso8601() mention IsoFormat.Normal,
>     rather than IsoFormat.Basic.

Oops. An oversight. Originally I named the IsoFormat values as Normal and  
Extended, but the ISO-8601 spec uses the terms Basic and Extended. I  
forgot to update the docs.

> 5. Is there any reason why you define the trivial opEquals() for
> DateCount?  I think the compiler takes care of that for you.

Unfortunately, if I remove the trivial opEquals (the one with the  
non-const parameter) the compiler complains when I compare a const  
DayCount with a non-cons one.

> I'm looking forward to seeing the time stuff as well!
>
> -Lars

Thanks again for the time you dedicated to do this review. I have set up a  
Bitbucket repo with this code at:  
http://bitbucket.org/gomez/yao-library/src

So whenever I do a change you can go there and review the modifications.

Cheers.

-- 
Yao G.