[phobos] datetime review (new attempt at URL)
Steve Schveighoffer
schveiguy at yahoo.com
Thu Oct 14 12:38:57 PDT 2010
OK, so I'll preface this by saying I've been out for the long weekend, then we
had a family emergency, so I've been away from this mailing list since last
Friday. So I haven't had a chance to look at many of the discusions on this
list about datetime (my unread messages on the Phobos list is 110 right now).
But as someone who's worked on datetime stuff in the past, I want to express my
opinions of this. I apologize if this version is completely out of date, or my
points have already been brought up.
So here goes:
I have read some of the other posts about the module size, and here is my
opinion: When you import a module, you don't care how many lines of code it is.
All you care about is getting access to a feature of that module. Often times,
more is imported than is necessary, so I think from that perspective, it doesn't
matter how big the module is. 40,000 LOC seems like a lot, but much of this is
very rote very boilerplate code, so I think the generated result is not going to
reflect that as much. When you go two levels deep, also remember that this
bloats the executable because the full module name is used to mangle all
symbols. Regarding docs, the doc generation is not a problem of having too
large modules, it's more of a problem of the doc generator not organizing the
output very well. Finally, in my experience, very few import the specific
modules they want, they just import everything. Most java programs don't import
java.io.xyz, they import java.io.*. What you are doing by splitting the code
into submodules is making it awkward to do this at best. I never really liked
the 'all' solution. My opinion is that you just have all the code in one
module, and not worry about the size.
----
I read the blurb from your original email about how everything is laid out.
Without looking at the code yet, I think all the limits and decisions seem OK.
A couple things that came up with my implementation of datetime on Tango:
1. Are you going to use an extended Gregorian calendar, or use a Julian calendar
for the appropriate dates? I recommend using extended Gregorian, because it's
much easier to deal with, and anyone who wants to deal with such historic
accuracy should be using a much more complete library. In any case, it should
be noted what you are using.
2. I highly recommend ignoring the concept of leap-seconds, as it just adds
constant maintenance (since leap seconds cannot be predicted) and doesn't add
much to the library. However, it should be noted whether you support them.
----
I don't like all the aliases for Year, etc. This accomplishes almost nothing
except documentation. And even that doesn't add much. I don't see the point of
doing:
int foo(Year years)
vs.
int foo(short years)
Both seem equally documented to me.
----
I like the to!(TUnit, TUnit) conversion function, but I think it is redundant to
also have daysTohnsecs (btw, this isn't properly cased, I think it should have
been daysToHnsecs). I see the first uses the other, but a better implementation
is possible without requiring all the others. Also, you risk unnecessary
truncation in your calculations.
I'd say get rid of all the extra functions. I would also renumber the enum for
TUnit to go from smallest to largest, and I would rewrite the to conversions as:
template hnsecPer!(TUnit un) if(TUnit >= TUnit.week) // note reverse this if you
reorder enum
{
static if(un == TUnit.hnsec)
enum hnsecPer = 1L;
else static if(un == TUnit.usec)
enum hnsecPer = 10L;
else static if(un == TUnit.msec)
enum hnsecPer = 1000 * hnsecPer!TUnit.usec;
else static if(un == TUnit.second)
enum hnsecPer = 1000 * hnsecPer!TUnit.msec;
else static if(un == TUnit.minute)
enum hnsecPer = 60 * hnsecPer!TUnit.second;
else static if(un == TUnit.hour)
enum hnsecPer = 60 * hnsecPer!TUnit.minute;
else static if(un == TUnit.day)
enum hnsecPer = 24 * hnsecPer!TUnit.hour;
else static if(un == TUnit.week)
enum hnsecPer = 7 * hnsecPer!TUnit.day;
else static assert(0);
}
...
static long to(TUnit tuFrom, TUnit tuTo)(long value) pure nothrow
if(tuFrom >= TUnit.week && tuFrom <= TUnit.hnsec &&
tuTo >= TUnit.week && tuTo <= TUnit.hnsec)
{
static if(tuFrom > tuTo)
return value * (hnsecPer!tuTo / hnsecPer!tuFrom);
else
return value / (hnsecPer!tuFrom / hnsecPer!tuTo);
}
(Note -- untested)
----
toString!(TUnit) -- this seems like an extremely fringe need. Not often do I
care about printing a value representing seconds. More often I care about
printing a duration, time of day, or a date. Can we drop this and functions
that depend on it?
Also note that string representation of date/time is one of those things that is
highly sensitive to locale. Tango has a ginormous library dedicated to printing
locale-dependent stuff including date/time.
I agree that having a default print for date/time is fine, esp. for debugging,
but let's not try to reinvent formatted printing in the date time module.
----
timeT2StdTime and stdTime2TimeT -- I really don't like the names here. Can we
call it C Time? T has such a known usage as representing a generic type, this
was my immediate thought of what it does. In Tango, we called it toUnixTime and
fromUnixTime. Also, don't abbreviate to as 2. I hate that :)
Also, I don't think we need another version of this
(fromTimeTEpoch2StdTimeEpoch), if you have the wrong unit, just use the to!
functions defined above.
----
I'll echo what I've read from Andrei -- I don't like using classes as
namespaces. Find another way.
----
Encountered more string processing functions in core.d. Can we agree to print a
date/time out like this:
[mm/dd/yyyy] [hh:mm:ss.ffffffff]
Where mm is month, dd is day, yyyy is 4-digit year, hh is 24-hour hour, mm is
minute, ss is second, and ffffff is fractional seconds.
And for durations, we should come up with a similar format. C# I think uses
ddd.hh:mm:ss.ffffff where ddd is the total number of days.
If we can do this, then I think we can leave the pretty-printing of anything
else to a locale-based formatting library. People are going to want to use
their own locale anyways.
----
General nitpick comment, your ddoc is over 80 chars wide (over 100 in some
spots), can you fix this? I don't mind code being wider than necessary, but
comments should fit within an 80xN terminal.
----
In e.g. HNSecDuration.msecs, you are using a very convoluted way to get the
number of milliseconds :) Use mod instead.
And in general, can we just use a template? If we continuously parameterize
everything based on the unit type, generic programming is going to be much
easier.
i.e.
@property long get(TUnit unit type)() {...}
alias get!(TUnit.msec) msecs;
...
----
I think we also need totalX where X is Msecs, Secs, etc. For instance, you may
only care about how many days you have, and not how many weeks. So to get the
total number of days, you would currently have to do x.weeks * 7 + x.days. If
you wanted the total milliseconds, it would be worse.
This can also be a generic template with aliases.
----
JointDuration -- aside from operators, do we need to wrap the other methods of
HNSecDuration and MonthDuration? Can we just provide accessors for the
MonthDuration and HNSecDuration (in fact, you may want this).
----
TimeOfDay: Can we use HNSecDuration with an invariant that it's < 24 hours? I
can't see why you'd want to reimplement all this. FWIW, Tango uses Span (the
duration type) as it's timeofday component.
Date is one thing -- the durations are based on a point in time. But time of
day is always the same no matter the day.
I'd expect the following structs in timepoint.d:
Date -- a date with the fields year month day
Time -- A HNSecDuration with the limitation that it must be less than 24 hours
DateTime -- a combination of both Date and Time
As far as time zone, I've not yet dealt with it. It was on my plate to add time
zones to Tango, but I never got around to it. I think a type that combines a
point in time with a time zone might be the best solution, similar to how an
interval combines a point in time with a duration.
You have other time types in timepoint which I think are not necessary. Like
FracSec.
---
OK, so that's what I have. I think it's a very well thought out lib, it just
needs to be trimmed down.
One final thought -- after reading through all the stuff, unit tests take up the
vast bulk of the lines of code. I think it's safe to say the .di file would be
like 5000 LOC. I think it definitely should be one file.
-Steve
----- Original Message ----
> From: Jonathan M Davis <jmdavisProg at gmx.com>
> To: phobos at puremagic.com
> Sent: Fri, October 8, 2010 5:59:17 PM
> Subject: Re: [phobos] datetime review (new attempt at URL)
>
> Okay. Hopefully this URL works:
>
> http://is.gd/fS35q
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos
>
More information about the phobos
mailing list