Refactor Date.

Refactor Date.

[Sean Eagan, 2012-05-14 14:43:51]

LGTM (I always wanted to do that ;) ) with comments

https://chromiumcodereview.appspot.com/10384149/diff/4001/corelib/src/date.dart
File corelib/src/date.dart (right):

https://chromiumcodereview.appspot.com/10384149/diff/4001/corelib/src/date.da...
corelib/src/date.dart:19: int get timeZoneOffsetInMinutes()
this should return a Duration instead of hard coded to minutes.  A good name
would be 'utcOffset'.

https://chromiumcodereview.appspot.com/10384149/diff/4001/corelib/src/date.da...
corelib/src/date.dart:22: bool get summerTimeSavingsIsActive()
it looks like this tries to combine the terms 'daylight savings time' and
'summer time' into one.  I think it would be better to just pick one and go with
it.  Also, I think name should have an 'is' prefix, e.g.:

isDaylightTime
isDaylightSavingsTime
isSummerTime
isDST (abbreviation as a last resort)

https://chromiumcodereview.appspot.com/10384149/diff/4001/corelib/src/date.da...
corelib/src/date.dart:150: String toUTCString();
toUtcString

per the Dart style guide

[floitsch, 2012-05-14 15:01:16]

Thanks for the comments.

https://chromiumcodereview.appspot.com/10384149/diff/4001/corelib/src/date.dart
File corelib/src/date.dart (right):

https://chromiumcodereview.appspot.com/10384149/diff/4001/corelib/src/date.da...
corelib/src/date.dart:19: int get timeZoneOffsetInMinutes()
On 2012/05/14 14:43:51, seaneagan1 wrote:
> this should return a Duration instead of hard coded to minutes.  A good name
> would be 'utcOffset'.

good catch.

https://chromiumcodereview.appspot.com/10384149/diff/4001/corelib/src/date.da...
corelib/src/date.dart:22: bool get summerTimeSavingsIsActive()
On 2012/05/14 14:43:51, seaneagan1 wrote:
> it looks like this tries to combine the terms 'daylight savings time' and
> 'summer time' into one.  I think it would be better to just pick one and go
with
> it.  Also, I think name should have an 'is' prefix, e.g.:
> 
> isDaylightTime
> isDaylightSavingsTime
> isSummerTime
> isDST (abbreviation as a last resort)
> 

my mistake. I wanted to write DaylightSavingsTimeIsActive.
The problem with "isDaylightSavingsTime" is that, for me, the name sounds like
it should be on a Date. Here we ask if the timzone is in daylightsavings mode.

https://chromiumcodereview.appspot.com/10384149/diff/4001/corelib/src/date.da...
corelib/src/date.dart:148: * Example: [:2012-05-14 12:21:04.989Z:]
Should we instead return a valid ISO 8601? It would only require to replace the
space with a "T":
2012-05-14T12:21:04.989Z

https://chromiumcodereview.appspot.com/10384149/diff/4001/corelib/src/date.da...
corelib/src/date.dart:150: String toUTCString();
On 2012/05/14 14:43:51, seaneagan1 wrote:
> toUtcString
> 
> per the Dart style guide

Done.

[floitsch, 2012-05-14 15:35:16]

CL implementing Hashable.
https://chromiumcodereview.appspot.com/10391109/

[Sean Eagan, 2012-05-14 16:05:16]

https://chromiumcodereview.appspot.com/10384149/diff/4001/corelib/src/date.dart
File corelib/src/date.dart (right):

https://chromiumcodereview.appspot.com/10384149/diff/4001/corelib/src/date.da...
corelib/src/date.dart:22: bool get summerTimeSavingsIsActive()
On 2012/05/14 15:01:16, floitsch wrote:
> The problem with "isDaylightSavingsTime" is that, for me, the name sounds like
> it should be on a Date. Here we ask if the timzone is in daylightsavings mode.

oh, I thought this was an instance method on Date... but shouldn't it be ?  This
basically tells whether the current Date (i.e. new Date.now()) is in daylight
savings time, whereas it seems that would be useful to know for _any_ Date.

If it were to remain static, I wonder whether it should be a static method on
Date as opposed to a top-level.

In any case I still think 'isDaylightSavingsTime' is a better name, but maybe it
wouldn't be used enough to be a big deal anyway.

[floitsch, 2012-05-14 16:12:11]

https://chromiumcodereview.appspot.com/10384149/diff/4001/corelib/src/date.dart
File corelib/src/date.dart (right):

https://chromiumcodereview.appspot.com/10384149/diff/4001/corelib/src/date.da...
corelib/src/date.dart:22: bool get summerTimeSavingsIsActive()
On 2012/05/14 16:05:16, seaneagan1 wrote:
> On 2012/05/14 15:01:16, floitsch wrote:
> > The problem with "isDaylightSavingsTime" is that, for me, the name sounds
like
> > it should be on a Date. Here we ask if the timzone is in daylightsavings
mode.
> 
> oh, I thought this was an instance method on Date... but shouldn't it be ? 
This
> basically tells whether the current Date (i.e. new Date.now()) is in daylight
> savings time, whereas it seems that would be useful to know for _any_ Date.
Yes and no. It is both a property of a specific Date, as well as a property of
the machine. Basically the same as the Timezone itself.

> 
> If it were to remain static, I wonder whether it should be a static method on
> Date as opposed to a top-level.
That's where I wanted it to be, but Date is an interface and interfaces are not
allowed to have static methods.
> 
> In any case I still think 'isDaylightSavingsTime' is a better name, but maybe
it
> wouldn't be used enough to be a big deal anyway.
Given that I don't see how to implement this in JS anyways I'm dropping this
predicate for now.

[floitsch, 2012-05-15 13:14:20]

Operator CL:
https://chromiumcodereview.appspot.com/10399027