Re: datetime review part 2 [Update 4]
Jonathan M Davis Wrote: If you have a better way to do it, I'm all ears. However, it's the only way that I know of to get high-precision time on Windows. Use GetSystemTime. At least, it will be correct.
Re: datetime review part 2 [Update 4]
On Thursday 18 November 2010 03:35:49 Kagamin wrote: Jonathan M Davis Wrote: If you have a better way to do it, I'm all ears. However, it's the only way that I know of to get high-precision time on Windows. Use GetSystemTime. At least, it will be correct. Actually, looking at TickDuration, for the stopwatch stuff, it really should be dealing with monotonic time. So, it should probably use the monotonic clock on Linux rather than the real time clock, and currTime() should use a different set of code that uses the real time clock on Linux and whatever the best solution is on Windows. I'll play around with GetFileSystemTime() to see whether it actually works like it's supposed to, or if it still has the problems mentioned in the article that you linked to. - Jonathan M Davis
Re: datetime review part 2 [Update 4]
Jonathan M Davis Wrote: Latest: http://is.gd/gSwDv You use QueryPerformanceCounter. Is this code tested on Windows? MSDN doesn't specify what QueryPerformanceCounter returns. see http://msdn.microsoft.com/en-us/magazine/cc163996.aspx
Re: datetime review part 2 [Update 4]
On Wednesday, November 17, 2010 13:44:32 Kagamin wrote: Jonathan M Davis Wrote: Latest: http://is.gd/gSwDv You use QueryPerformanceCounter. Is this code tested on Windows? MSDN doesn't specify what QueryPerformanceCounter returns. see http://msdn.microsoft.com/en-us/magazine/cc163996.aspx SHOO wrote that portion of the code as part of his stopwatch stuff, so I'm probably not as clear on that as I am on most of the other stuff, but it was working for him, as far as I know, and I've done some testing on wine (including making sure that all of the unit tests pass), so it's not like it blows up or anything like that. There could be some sort of fundamental problem with it or subtle bug that I'm not aware of, but it at least appears to work. I'd have to study up on it to see whether there are any real problems with it. I was just using the code that SHOO had to get the current system time at high resolution. I figured that he'd figured all that out, and there was no reason to duplicate effort. - Jonathan M Davis
Re: datetime review part 2 [Update 4]
The article was written in 2004. A high precision event timer has been incorporated in chipsets since 2005. http://en.wikipedia.org/wiki/High_Precision_Event_Timer I hope were not basing decisions on support for NT4.0 :) == Quote from Kagamin (s...@here.lot)'s article Jonathan M Davis Wrote: Latest: http://is.gd/gSwDv You use QueryPerformanceCounter. Is this code tested on Windows? MSDN doesn't specify what QueryPerformanceCounter returns. see http://msdn.microsoft.com/en-us/magazine/cc163996.aspx
Re: datetime review part 2 [Update 4]
On Wednesday 17 November 2010 16:09:22 Todd VanderVeen wrote: The article was written in 2004. A high precision event timer has been incorporated in chipsets since 2005. http://en.wikipedia.org/wiki/High_Precision_Event_Timer I hope were not basing decisions on support for NT4.0 :) I'm sure not. I believe that most or all of the Windows system calls that are made in std.datetime date back to Win2k. WindowsTimeZone could be improved if I could assume that Windows was Vista or newer, but that's obviously not reasonable at this point, so I used the Win2k functions for getting time zone information (the main difference being that the new ones can get correct DST info for historical dates whereas the old ones only ever use the current DST rules). I believe that the general philosophy is to support the oldest Windows OS that is reasonable (so, for example, if you can do it one way and support back to Win98 and another way which would support to Win2K and they're pretty much equal as far as utility or complexity goes, then choose the Win98 way). I don't know what the upper limit is though. XP obviously has to be supported, so anything newer than that is automatically out, but I don't know if a system function which was added in XP would be okay or not. Regardless, std.datetime assumes that your version of Windows is at least Win2k but does not assume that it's newer than that. - Jonathan M Davis
Re: datetime review part 2 [Update 4]
Jonathan M Davis Wrote: I'd have to study up on it to see whether there are any real problems with it. Speaking in posix terms, performance counter is more like CLOCK_MONOTONIC and using it as CLOCK_REALTIME is a dependency on undefined behavior.
Re: datetime review part 2 [Update 4]
It's difficult to find a suitable entry point in this thread, so I'll just arbitrarily use here. Various language libraries have flexible facilities for formatting date/time values, maybe c#, and certainly PHP, whereby you can specify a format string, something like %d'th %M %Y. Is this a useful idea? I have some code kicking around somewhere. Steve
Re: datetime review part 2 [Update 4]
On Wednesday 17 November 2010 21:57:58 Steve Teale wrote: It's difficult to find a suitable entry point in this thread, so I'll just arbitrarily use here. Various language libraries have flexible facilities for formatting date/time values, maybe c#, and certainly PHP, whereby you can specify a format string, something like %d'th %M %Y. Is this a useful idea? I have some code kicking around somewhere. It is definitely a useful idea. However, having strings in standard formats is more important (and far simpler - albeit not simple), so that's what's included at the moment. At some point, I will likely implement toString() and fromString() methods which take format strings, but having those be appropriately flexible and getting them right is not simple, so I put them off for the moment. I have several TODO comments in the code with ideas of functionality to add at a later date, and that is one of them. So, ideally it would be in there, but it already took long enough to implement what's there that it seemed appropriate to put off implementing further functionality until the basic design is approved and in Phobos. - Jonathan M Davis
Re: datetime review part 2 [Update 4]
On Wednesday 17 November 2010 21:51:24 Kagamin wrote: Jonathan M Davis Wrote: I'd have to study up on it to see whether there are any real problems with it. Speaking in posix terms, performance counter is more like CLOCK_MONOTONIC and using it as CLOCK_REALTIME is a dependency on undefined behavior. If you have a better way to do it, I'm all ears. However, it's the only way that I know of to get high-precision time on Windows. - Jonathan M Davis
Re: datetime review part 2 [Update 4]
On 14.11.2010 7:47, Jonathan M Davis wrote: On Thursday 11 November 2010 13:42:37 Dmitry Olshansky wrote: I'm afraid that I don't really get what you're trying to do here. A range needs to be created from an interval. It really wouldn't make sense to do it otherwise. And when you create a range, it needs a delegate which generates the time points for that range. So, you create an interval and call either fwdRange() or bwdRange() on it to give it the range-generating delegate. I happen to have provided some helper functions for generating delegates, but there's nothing to stop you from creating your own. If you have provided them, then it's OK. What I meant is we have retro(someRange) to go backwards where applicable. And I do believe you could return range of DateTime's that's bidirectional ? Then there is no need to duplicate the std.algorithm/range functionality, that's all. None of the ranges in std.datetime are bi-directional, so you can't use retro on them (or any other algorithm which requires bi-directional ranges). It's a side effect of them being generated as you iterate. I considered having bi-directional ranges, but it was just too messy. You'd have to provide two separate generative functions (one for each direction), and in many cases, when iterating over dates, you wouldn't get the same results because many date calculations aren't reversible (the main problem being that months and years don't have the same number of days). So bi-directionality is one of places where generator-on-previous-date approach is failing. I see that with current API the user need to be aware which delegate use when: auto func = IRange.everyDayOfWeek!(Date, Direction.bwd)(DayOfWeek.friday);//note the Direction.bwd bit auto range = interval.bwdRange(func);//also note the bwd thingy I get it would just throw exception if bwdRange got replaced by fwdRange? It's acceptable, but still not clean. Are you trying to say that the range should automatically provide _all_ possible time points in an interval and then you specifically filter them? That's nowhere near as flexible, and in the case of SysTime in particular, think about how many time points that is. It has hecto-nanosecond (100 ns) precision. That's 10 million time points a second. It could get really inefficient to try and filter all of those. Also, I'm not sure that filter would work with an infinite range (I'd have to check), which would be a huge setback. I really don't get the benefit of your suggestion, though I can undkerstand it if it's not entirely clear how ranges in std.datetime are supposed to work. Not at all, you provide the precision that the user asks for. E.g. if users want tot iterate by days - no problem iterate by days. If by hnseconds, well he(or she) knows better ... Right? The point is we don't need some predefined delegates, just the natural ones e.g. by secs, hours and so on as with the roll/add methods. That's doable, but it seems far less flexible. OK, got it. Skimming again through docs I'd say we just see the API at different sides, i.e. I was mostly referring to IRange block, it's slightly cumbersome. For now, I as user see it more as interval functions: Interval.byDur(Duration) + Interval.by!days(x) Interval.by!months(x) + Interval.byyears(x) where only byDur, bymonths and byyears are necessary the other are just a convenience. They could be implemented as one-liners in terms of yours IRange (I still don't get why such a name?). Then it would be just an implementation detail with IRange being private. But then it would cost in some extra lines, plus as you told there should be fwd/back param somewhere. Then it goes like Interval.by!(days,Direction.Back) with default being Fwd of course... For instance, how would you calculate each successive Easter? Or a holiday which always occurred on the Xth day of the week of a particular month? A number of calculations could easily be based on the dates that came before rather than examining each date on its own and determining whether it was one of the ones that you were looking to iterate over. Yeah, with that simplified approach using filtering by e.g. days could suck in term of speed, need checking. It's however possible, see my later point. If speed is of concern IMO I would just create a special range by hand, it's a standard library right? About speed: because of flexibility yours bwdRange and fwdRange are checking on both bounds(if any) at each iteration, for trivial tasks that's also costs. Maybe we need some realistic date-time benchmarks (if there is such a thing)? I mean, even iterating over each successive month with that wouldn't work very well. If the first date were 2010-07-31, would 2010-09-30 be one of the dates to iterate over? In order to know one way or the other, you'd need to have state (like what the last date to be iterated over was), and using a filter does not give you state. Filter based on delegate could have state, so no
Re: datetime review part 2 [Update 4]
On Sunday 14 November 2010 02:31:00 Dmitry Olshansky wrote: On 14.11.2010 7:47, Jonathan M Davis wrote: So bi-directionality is one of places where generator-on-previous-date approach is failing. I see that with current API the user need to be aware which delegate use when: auto func = IRange.everyDayOfWeek!(Date, Direction.bwd)(DayOfWeek.friday);//note the Direction.bwd bit auto range = interval.bwdRange(func);//also note the bwd thingy I get it would just throw exception if bwdRange got replaced by fwdRange? It's acceptable, but still not clean. It'll throw if you use Direction.bwd with interval.fwdRange() or Direction.fwd with interval.bwdRange(). And using the filter approach wouldn't really fix the bi-directional problem anyway, since you'd have to iterate over the entire range in order to determine what the time points at the end of the range are. Backwards ranges only work because they calculate from the end point rather than the begin point. They aren't going to necessarily result in the same time points as iterating forwards would. Even if you figured out how to deal with the day overflow issues (covered below) and state while using filter, you'd have the exact same problem where you'd have to iterate over _all_ of the time points to be able to know what the end was. Are you trying to say that the range should automatically provide _all_ possible time points in an interval and then you specifically filter them? That's nowhere near as flexible, and in the case of SysTime in particular, think about how many time points that is. It has hecto-nanosecond (100 ns) precision. That's 10 million time points a second. It could get really inefficient to try and filter all of those. Also, I'm not sure that filter would work with an infinite range (I'd have to check), which would be a huge setback. I really don't get the benefit of your suggestion, though I can undkerstand it if it's not entirely clear how ranges in std.datetime are supposed to work. Not at all, you provide the precision that the user asks for. E.g. if users want tot iterate by days - no problem iterate by days. If by hnseconds, well he(or she) knows better ... Right? The point is we don't need some predefined delegates, just the natural ones e.g. by secs, hours and so on as with the roll/add methods. That's doable, but it seems far less flexible. OK, got it. Skimming again through docs I'd say we just see the API at different sides, i.e. I was mostly referring to IRange block, it's slightly cumbersome. For now, I as user see it more as interval functions: Interval.byDur(Duration) + Interval.by!days(x) Interval.by!months(x) + Interval.byyears(x) where only byDur, bymonths and byyears are necessary the other are just a convenience. They could be implemented as one-liners in terms of yours IRange (I still don't get why such a name?). Then it would be just an implementation detail with IRange being private. But then it would cost in some extra lines, plus as you told there should be fwd/back param somewhere. Then it goes like Interval.by!(days,Direction.Back) with default being Fwd of course... I'm not sure that I get what you're trying to do here. You have an interval, and if you want to iterate it, you need a range over that interval. I take it that you're trying to have specific range functions on interval instead of using the more generic fwdRange() and bwdRange()? That's considerably less generic, and what happens if the interval that you're trying to iterate over doesn't have months in it (e.g. TimeOfDay)? It's certainly doable, but it's less generic. The interval types are intended to be generic and work over any type of time point. Also, they have no concept of precision across an interval, so whether you're dealing with time points months apart or hnsecs apart has nothing to do with interval as it stands. That's entirely up to the range. You could put that kind of information in the interval, but other than iterating over it (which is the range's job), that information is completely irrelevant. What I have is generic over any type of time point. You use fwdRange() or bwdRange() and give it the delegate to generate the range. IRange is there to provide you with delegates that you're likely to need without figuring out having to code them yourself. IRange stands for IntervalRange, and it would be _useless_ if it were not a public API. The entire point of it is to save users from having to declare the delegates themselves. It gives basic delegates for some of the common situations (though it would probably be good to add a few more). It's not quite as clean as I'd like because it doesn't know whether you intend to use a delegate with fwdRange() or bwdRange(), so in the case of bwdRange(), you have to pass Direction.bwd to them, but it works, and I'd expect people to want to iterate forwards most of the time anyway. The
Re: datetime review part 2 [Update 4]
On Thursday 11 November 2010 13:42:37 Dmitry Olshansky wrote: I'm afraid that I don't really get what you're trying to do here. A range needs to be created from an interval. It really wouldn't make sense to do it otherwise. And when you create a range, it needs a delegate which generates the time points for that range. So, you create an interval and call either fwdRange() or bwdRange() on it to give it the range-generating delegate. I happen to have provided some helper functions for generating delegates, but there's nothing to stop you from creating your own. If you have provided them, then it's OK. What I meant is we have retro(someRange) to go backwards where applicable. And I do believe you could return range of DateTime's that's bidirectional ? Then there is no need to duplicate the std.algorithm/range functionality, that's all. None of the ranges in std.datetime are bi-directional, so you can't use retro on them (or any other algorithm which requires bi-directional ranges). It's a side effect of them being generated as you iterate. I considered having bi-directional ranges, but it was just too messy. You'd have to provide two separate generative functions (one for each direction), and in many cases, when iterating over dates, you wouldn't get the same results because many date calculations aren't reversible (the main problem being that months and years don't have the same number of days). Are you trying to say that the range should automatically provide _all_ possible time points in an interval and then you specifically filter them? That's nowhere near as flexible, and in the case of SysTime in particular, think about how many time points that is. It has hecto-nanosecond (100 ns) precision. That's 10 million time points a second. It could get really inefficient to try and filter all of those. Also, I'm not sure that filter would work with an infinite range (I'd have to check), which would be a huge setback. I really don't get the benefit of your suggestion, though I can undkerstand it if it's not entirely clear how ranges in std.datetime are supposed to work. Not at all, you provide the precision that the user asks for. E.g. if users want tot iterate by days - no problem iterate by days. If by hnseconds, well he(or she) knows better ... Right? The point is we don't need some predefined delegates, just the natural ones e.g. by secs, hours and so on as with the roll/add methods. That's doable, but it seems far less flexible. For instance, how would you calculate each successive Easter? Or a holiday which always occurred on the Xth day of the week of a particular month? A number of calculations could easily be based on the dates that came before rather than examining each date on its own and determining whether it was one of the ones that you were looking to iterate over. I mean, even iterating over each successive month with that wouldn't work very well. If the first date were 2010-07-31, would 2010-09-30 be one of the dates to iterate over? In order to know one way or the other, you'd need to have state (like what the last date to be iterated over was), and using a filter does not give you state. It seems to me that using a filter could work well in simple cases but that it falls apart in more complex ones. Well, I'm not saying it's bad/or some such. I, first of all, told you it's damn good. Things could be simplified though if you relay some of burden on std.algorithm/range and such. That's the main point I couldn't get for the moment. I think that the real concern is whether the ranges in std.datetime can be used with all of the algorithms in std.range and std.algorithm that they're likely to be want to used with. As they stand, they aren't bi-directional, which could be a problem, but I don't see a good way to make them bi-directional, since date calculations aren't always reversible. You also can't swap them at all, but they have a fixed order by their very nature, so I don't see that as being a problem. There could be problems that I'm not seeing though. Regardless, I don't see std.algorithm or std.range as being much help in generating ranges in std.datetime. It might be nice to use retro to produce ranges which iterate into the past, but the lack of reversibility of date calculations makes it so that doesn't work. And in most cases, a delegate which calculates the next date in the range given the previous one is quite straightforward - if it isn't, it's because the calculation itself is complicated, and attempts at using filter and the like won't make the calculation any simpler. - Jonathan M Davis
Re: datetime review part 2 [Update 4]
Jonathan M Davis napisał: On Thursday, November 11, 2010 16:31:56 Tomek Sowiński wrote: That would require you to be able to have unary +. I didn't think that that was legal. Upon checking it out though, it looks like it is. I don't really have any problem with the way that I did it, but I can see where you're coming from. I'll look at reducing some of those functions in a manner similar to that. Thanks. BTW, I still think units of time should be an enum, not a string. No, strings are definitely better. I had an enum originally, and it was unusable in comparison. The main problem is that you have to always preface enum values with the enum name - either that you create an anonymous enum, but then you've got the problem that incredibly common terms such as years and seconds are now useless as variable names. The strings were Andrei's suggestion, and I think that they're a definite improvement. It's a bit harder to work with them internally in some places (such as when dealing with one unit being greater than another), but it's definitely a net gain. Interesting. I've always preferred enums but what you're saying does make sense. I'm curious how it pans out. Well, I can't really include both long and short names very cleanly in a named enum (especially when there's code that relies on the order and value of the values). It really should be one or the other, though I'm open to using the short names rather than the long names for the days of the week. If I had to choose one convention, it'd be it. Literals rarely come up in production code, more likely in unittests where clipping them short makes sense. Designing the interface and, more importantly, solving how to store the holiday information to allow fast interval querying is not locale-specific. Plus, it is in common need for business, and it is not trivial -- ideal candidate for the standard library. Then everyone can write an implementation for their favorite country or stock exchange on their own. I'm certainly not against adding such functionality to std.datetime, but I hadn't considered it, and I think that what I have needs to be appropriately refined and then included into Phobos before looking at adding major new functionality. I would like to add stuff like date recurrence patterns in the future, and if there is any commonly useful date/time stuff which isn't too localized, adding it to std.datetime could make a lot of sense, but I'd like the base done first. Amen. Dates are priority. Then we'll think of higher-level structures/operations. Besides, I haven't heard of a framework which has business day/holiday calculation really figured out, so the design will probably have to be done from scratch. -- Tomek
Re: datetime review part 2 [Update 4]
On 11.11.2010 8:55, Jonathan M Davis wrote:
On Wednesday 10 November 2010 10:18:27 Dmitry Olshansky wrote:
OK, I looked through the docs, to me it's very solid. I fact I never
written any code that heavily uses date-time stuff, most of the time I
just converted everything to UNIX timestamp, and back to printable form,
once the processing is done.
One suggestion about docs - since SysTime and DateTime have similar API,
wouldn't it be better to merge documentation
and represent it in form of a table (like the one in std.container,
unfortunately ATM it's screwed up )?
In the same table you could also place functions specific for SysTime,
just add a remark to it.
Another one - group overloads together, consider for instance
std.algorithm. It doesn't leak all of it's template specializations for
all kinds of ranges with the same ddoc comment. You shouldn't i.e.
const pure bool_intersects_(in Interval/interval/);
Whether the given/interval/overlaps with this/interval/.
const pure bool_intersects_(in PosInfInterval!(TP)/interval/);
Whether the given/interval/overlaps with this/interval/.
...
It only could get you tired - there is no new information at all.
May I also humbly suggest you create the second table describing generic
Interval notion (with specifics fro infinite ones marked as such).
I'm not sure the best way to do all of that. std.container tries to give a
general overview of container capabilities at the beginning and then still has
all of the specific functions listed for each type still. I could try something
like that, though making that sort of change is not something that I'm likely to
get to very quickly. It'll take a fair bit of thinking and experimentation to
come up with the best way to do that. As for specific overloads and
documentation, I'm not sure what the best way to handle that is, but it does
need to be made clear which functions work with which types. Taking the interval
functions for instant, some of them - like intersection - work with all interval
types regardless of the interval type that you're dealing with, while others -
like union - only work with a subset of the interval types (since things like
the union between positively and negatively infinite intervals make no sense).
What I likely need to do is try and come up with better documentation up front
which better gets across the basic groups of types (time points, intervals,
etc.) and their basic abilities while leaving the individual documentation to
still give all of the details that it needs to. I'll look into doing that later,
but I can't say that it's a high priority at the moment. Good suggestion though.
And one more thing about the _API_ itself:
auto interval = Interval!Date(Date(2010, 9, 2), Date(2010, 9, 27));
auto func = IRange.everyDayOfWeek!Date(DayOfWeek.monday);
auto range = interval.fwdRange(func);
I'd preffer more simple and generic things, they tend to combine with
std.algorothm so nicely (It's just an idea worth researching).
IMHO also could remove a ton of code from date-time, that do not belong
here anyway.
My try at the code above would be:
auto interval = Interval!Date(Date(2010, 9, 2), Date(2010, 9, 27));
auto onlyMondays = filter!((x){ return x.dayOfWeek == DayOfWeek.monday });
auto range = onlyMondays (interval.by!days); //reuses the same
convention for add and roll, more coherent
I'm afraid that I don't really get what you're trying to do here. A range needs
to be created from an interval. It really wouldn't make sense to do it
otherwise. And when you create a range, it needs a delegate which generates the
time points for that range. So, you create an interval and call either
fwdRange() or bwdRange() on it to give it the range-generating delegate. I
happen to have provided some helper functions for generating delegates, but
there's nothing to stop you from creating your own.
If you have provided them, then it's OK.
What I meant is we have retro(someRange) to go backwards where applicable.
And I do believe you could return range of DateTime's that's
bidirectional ?
Then there is no need to duplicate the std.algorithm/range
functionality, that's all.
Are you trying to say that the range should automatically provide _all_ possible
time points in an interval and then you specifically filter them? That's nowhere
near as flexible, and in the case of SysTime in particular, think about how many
time points that is. It has hecto-nanosecond (100 ns) precision. That's 10
million time points a second. It could get really inefficient to try and filter
all
of those. Also, I'm not sure that filter would work with an infinite range (I'd
have to check), which would be a huge setback. I really don't get the benefit of
your suggestion, though I can undkerstand it if it's not entirely clear how
ranges in std.datetime are supposed to work.
Not at all, you provide the precision that the user asks for. E.g. if
users want tot iterate by days - no problem iterate by days.
If by hnseconds, well
Re: datetime review part 2 [Update 4]
Jonathan M Davis napisał:
On Wednesday, November 10, 2010 15:03:11 Tomek Sowiński wrote:
Jonathan M Davis napisał:
Latest: http://is.gd/gSwDv
My 2 cents:
Units of time are represented more naturally by an integer enum (could be
anonymous) than a string.
A problem recurring in many methods:
/+ref+/ Date opOpAssign(string op, D)(in D duration) nothrow
if((op == + || op == -)
(is(Unqual!D == Duration) ||
is(Unqual!D == TickDuration)))
{
static if(op == +)
{
static if(is(Unqual!D == Duration))
return addDays(convert!(hnsecs,
days)(duration.total!hnsecs)); else static if(is(Unqual!D ==
TickDuration))
return addDays(convert!(hnsecs,
days)(duration.hnsecs)); }
else
{
static if(is(Unqual!D == Duration))
return addDays(convert!(hnsecs,
days)(-duration.total!hnsecs)); else static if(is(Unqual!D ==
TickDuration))
return addDays(convert!(hnsecs,
days)(-duration.hnsecs)); }
}
If you're static if'ing each statement, it's a good sign the method
should be broken up into two overloads. BTW, what was the problem with
returning by ref?
I don't see any real benefit in splitting a function into two overloads
rather than using static ifs in this manner. The resulting source code is
likely nearly identical. You only end up with one function to document
this way, and in this case the user doesn't necessarily care about what
type of duration they're dealing with. If, they want to add or subtract a
duration, here's the function to do it. If anything, I think that I tend
to favor using static ifs over duplicate functions with different template
constraints if there's no real difference in the from the users
perspective.
Fair enough, although it's no excuse for copy-paste programming. It can be
easily wrung out of
duplication with no loss in readability:
/+ref+/ Date opOpAssign(string op)(in Duration duration) nothrow
if((op == + || op == -)
(is(Unqual!D == Duration) ||
is(Unqual!D == TickDuration)))
{
static if (is(Unqual!D == Duration))
auto hnecs = duration.total!hnsecs;
else
auto hnecs = duration.hnsecs;
return addDays(convert!(hnsecs, days)(unaryFun!(op~a)(hnsecs)));
}
The user may not care, but the maintainer does. Not even convinced about the
former, when writing I
often seek introspection in Phobos source. Besides, this is the Standard
Library, it should hold code
of exemplary quality.
BTW, I still think units of time should be an enum, not a string.
As for the ref, there are several bugs relating to that. One I remember
right off the top of my head is that you can't have a template function
return by ref for some reason. I think that there's at least one related
to invariants too, though the one about invariants not being able to be
pure has been fixed which has reduced some of the invariant-related
problems. A number of bugs relating to const-correctness, purity, and
invariants have caused me a lot of headaches when working on datetime.
Oh yeah, I know how that hurts ;)
Finally, bordering on bikeshed, weekdays' names are long and months' are
short. Having both short and long names for weekdays and months would
solve the inconsistency and satisfy everyone.
I hadn't thought about that. I'll think about it. I'm not sure that it
really matters much though. I tend to prefer full names over short names,
but that can become tedious when you use them a lot.
3-letter shorts, e.g. January-Jan, February-Feb, Monday-Mon, Tuesday-Tue are
recognized by just about
everyone. It makes sense to include both.
Are there any plans for a Calendar interface to allow user
implementations that distinguish business days and holidays?
I'm afraid that I have no clue what you're talking about. I mean, I know
what a business day is and what a holiday is, but I'm not sure what you
mean by a calendar interface in this context.
Sorry, was talking QuantLib, I meant something like this:
http://dsource.org/projects/quantlibd/browser/ql/time/calendars.d
(copy-pasting sucks, I know;))
Are you looking for a way to
query whether a particular day is a business day, weekend day, or holiday?
That strikes me as being a function rather than an interface
An interface (or abstract class) makes sense. E.g. I may want to get the number
of working days
between this and this date. It may be implemented faster than walking the
interval and pecking day
by day.
and it would
be locale-dependent enough that I can't see it making into the standard
library.
Designing the interface and, more importantly, solving how to store the holiday
information to allow
fast interval querying is not locale-specific. Plus, it is in common need for
business, and it is not trivial
-- ideal candidate for the standard library. Then
Re: datetime review part 2 [Update 4]
On Thursday, November 11, 2010 16:31:56 Tomek Sowiński wrote:
Jonathan M Davis napisał:
I don't see any real benefit in splitting a function into two overloads
rather than using static ifs in this manner. The resulting source code is
likely nearly identical. You only end up with one function to document
this way, and in this case the user doesn't necessarily care about what
type of duration they're dealing with. If, they want to add or subtract a
duration, here's the function to do it. If anything, I think that I tend
to favor using static ifs over duplicate functions with different
template constraints if there's no real difference in the from the users
perspective.
Fair enough, although it's no excuse for copy-paste programming. It can be
easily wrung out of duplication with no loss in readability:
/+ref+/ Date opOpAssign(string op)(in Duration duration) nothrow
if((op == + || op == -)
(is(Unqual!D == Duration) ||
is(Unqual!D == TickDuration)))
{
static if (is(Unqual!D == Duration))
auto hnecs = duration.total!hnsecs;
else
auto hnecs = duration.hnsecs;
return addDays(convert!(hnsecs,
days)(unaryFun!(op~a)(hnsecs))); }
The user may not care, but the maintainer does. Not even convinced about
the former, when writing I often seek introspection in Phobos source.
Besides, this is the Standard Library, it should hold code of exemplary
quality.
That would require you to be able to have unary +. I didn't think that that was
legal. Upon checking it out though, it looks like it is. I don't really have
any
problem with the way that I did it, but I can see where you're coming from.
I'll
look at reducing some of those functions in a manner similar to that.
BTW, I still think units of time should be an enum, not a string.
No, strings are definitely better. I had an enum originally, and it was
unusable
in comparison. The main problem is that you have to always preface enum values
with the enum name - either that you create an anonymous enum, but then you've
got the problem that incredibly common terms such as years and seconds are now
useless as variable names. The strings were Andrei's suggestion, and I think
that they're a definite improvement. It's a bit harder to work with them
internally in some places (such as when dealing with one unit being greater
than
another), but it's definitely a net gain.
Finally, bordering on bikeshed, weekdays' names are long and months' are
short. Having both short and long names for weekdays and months would
solve the inconsistency and satisfy everyone.
I hadn't thought about that. I'll think about it. I'm not sure that it
really matters much though. I tend to prefer full names over short names,
but that can become tedious when you use them a lot.
3-letter shorts, e.g. January-Jan, February-Feb, Monday-Mon, Tuesday-Tue
are recognized by just about everyone. It makes sense to include both.
Well, I can't really include both long and short names very cleanly in a named
enum (especially when there's code that relies on the order and value of the
values). It really should be one or the other, though I'm open to using the
short names rather than the long names for the days of the week.
Are there any plans for a Calendar interface to allow user
implementations that distinguish business days and holidays?
I'm afraid that I have no clue what you're talking about. I mean, I know
what a business day is and what a holiday is, but I'm not sure what you
mean by a calendar interface in this context.
Sorry, was talking QuantLib, I meant something like this:
http://dsource.org/projects/quantlibd/browser/ql/time/calendars.d
(copy-pasting sucks, I know;))
Are you looking for a way to
query whether a particular day is a business day, weekend day, or
holiday? That strikes me as being a function rather than an interface
An interface (or abstract class) makes sense. E.g. I may want to get the
number of working days between this and this date. It may be implemented
faster than walking the interval and pecking day by day.
and it would
be locale-dependent enough that I can't see it making into the standard
library.
Designing the interface and, more importantly, solving how to store the
holiday information to allow fast interval querying is not
locale-specific. Plus, it is in common need for business, and it is not
trivial -- ideal candidate for the standard library. Then everyone can
write an implementation for their favorite country or stock exchange on
their own.
I'm certainly not against adding such functionality to std.datetime, but I
hadn't considered it, and I think that what I have needs to be appropriately
refined and then included into Phobos before looking at adding major new
functionality. I would like to add stuff like date recurrence patterns in the
future, and if
Re: datetime review part 2 [Update 4]
On Tue, 09 Nov 2010 18:48:30 -0500, bearophile bearophileh...@lycos.com wrote: Yao G.: Ugh. The datetime.d file is 1.5 MB? 0_o So it will be very well tested :) Look, the huge benefit of unit tests are that they are inline with the code. Either you want that or you want small files, I don't understand the expectation that the file should be small and compact as long as the implementation is... I 100% agree with Jonathan, just ignore the unit tests, and look at the API. (BTW, this is where a nice doc generator like the one Tango uses would come in handy -- it shows the implementation if you click on the function name). My suggestions: - Keep only 5-10% of the tests inside the datetime.d module, and move the other 90-95% in a separated datetime test module. - Use deterministic (repeatable) random data in a loop to perform some compact fuzzy testing - Use alias or string mixing to greatly reduce the amount of text contained in the tests. Tuples help a lot. All those lines may be replaced with a single loop that contains something like: assertEqual(SysTime(DateTime(z0, x1, x2, x3, x4, x5), FracSec.from!msecs(x6)).dayOfGregorianCal, x7) Where the xn data comes from an array of typecons.Tuples like: Data5(2010, 11, 1, 23, 59, 59, 999, 734_077) Well, I'm not certain that this is the reason, but calendars are riddled with corner cases. I would expect a well-tested date/time library to test all those corner cases. I know when writing the Date/Time code for Tango, there were quite a few odd cases to test. As long as the corner cases are tested, I think the rest of the fluff could possibly be reduced. However, on the other hand, unit tests cost nothing except to make the low-file-size purists cringe :) I say leave them all in, there are no requirements on the size and style of unit tests, just that they fully test the library. -Steve
Re: datetime review part 2 [Update 4]
Steven Schveighoffer: Well, I'm not certain that this is the reason, but calendars are riddled with corner cases. I would expect a well-tested date/time library to test all those corner cases. I have never suggested to remove unit tests. But in several languages and projects there is the convention of moving the tests in a separated place when they become very large. Currently the unittest support in D doesn't allow to do this this well. So I suggest to improve the way DMD manages unittests, to allow a handy and safe move of them in another module, when the programmer desires so (named unittests are probably a good starting point for this). Bye, bearophile
Re: datetime review part 2 [Update 4]
On 10.11.2010 4:52, Jonathan M Davis wrote:
On Tuesday, November 09, 2010 17:34:15 Jonathan M Davis wrote:
On Tuesday, November 09, 2010 16:23:56 Yao G. wrote:
On Tue, 09 Nov 2010 18:11:59 -0600, Jonathan M Davis
jmdavisp...@gmx.com
wrote:
I think that the real question here is how good the API is.
I think that the sheer size of the library, specially with a datetime.d
file with almost 31,600 lines of code (granted, most of them are unit
test and documentation), makes a little difficult to analyze or give a
proper review of the code. It will takes a fair share time to do it.
Maybe that's why very few people has given criticism or suggestions. I
only gave a cursory view (you need to scroll a lot just to find the
implementation code), but I'll give it a more throughly review tonight.
That's quite understandable, but that's part of the reason that the ddoc
html files are there. You don't actually have to go trolling through the
code to look at the API.
I should probably add that it would be worth it for folks to just look at the
documentation for what they would generally try and do with a datetime module
and see whether they can do it reasonably well and what problems they'd have.
While looking at the module as a whole is definitely needed, just having folks
look at the portions that they'd use and commenting on that would be useful. Is
getting the time easy enough and intuitive enough? Is doing what you need to do
with time in basic applications once you have it easy or intuitive enough? Etc.
- Jonathan M Davis
OK, I looked through the docs, to me it's very solid. I fact I never
written any code that heavily uses date-time stuff, most of the time I
just converted everything to UNIX timestamp, and back to printable form,
once the processing is done.
One suggestion about docs - since SysTime and DateTime have similar API,
wouldn't it be better to merge documentation
and represent it in form of a table (like the one in std.container,
unfortunately ATM it's screwed up )?
In the same table you could also place functions specific for SysTime,
just add a remark to it.
Another one - group overloads together, consider for instance std.algorithm.
It doesn't leak all of it's template specializations for all kinds of
ranges with the same ddoc comment. You shouldn't i.e.
const pure bool_intersects_(in Interval/interval/);
Whether the given/interval/overlaps with this/interval/.
const pure bool_intersects_(in PosInfInterval!(TP)/interval/);
Whether the given/interval/overlaps with this/interval/.
...
It only could get you tired - there is no new information at all.
May I also humbly suggest you create the second table describing generic
Interval notion (with specifics fro infinite ones marked as such).
And one more thing about the _API_ itself:
auto interval = Interval!Date(Date(2010, 9, 2), Date(2010, 9, 27));
auto func = IRange.everyDayOfWeek!Date(DayOfWeek.monday);
auto range = interval.fwdRange(func);
I'd preffer more simple and generic things, they tend to combine with
std.algorothm so nicely (It's just an idea worth researching).
IMHO also could remove a ton of code from date-time, that do not belong
here anyway.
My try at the code above would be:
auto interval = Interval!Date(Date(2010, 9, 2), Date(2010, 9, 27));
auto onlyMondays = filter!((x){ return x.dayOfWeek == DayOfWeek.monday });
auto range = onlyMondays (interval.by!days); //reuses the same
convention for add and roll, more coherent
P.S. [Nitpicking]
Some typos in docs:
int_diffMonths_(in Date/rhs/);
... You can get the difference in _yours_ by subtracting the year
property of two Dates,...
Should be years? Same thing in SysTime and DateTime, that just proves
my earlier point - documentations for these could be merged.
---
struct_SysTime_;
_SysTime_is the type used when you want _to_ the current time from
the system or if you're doing anything that involves time zones...
Should be to get?
[/Nitpicking]
--
Dmitry Olshansky
Re: datetime review part 2 [Update 4]
Jonathan M Davis napisał:
Latest: http://is.gd/gSwDv
My 2 cents:
Units of time are represented more naturally by an integer enum (could be
anonymous) than a string.
A problem recurring in many methods:
/+ref+/ Date opOpAssign(string op, D)(in D duration) nothrow
if((op == + || op == -)
(is(Unqual!D == Duration) ||
is(Unqual!D == TickDuration)))
{
static if(op == +)
{
static if(is(Unqual!D == Duration))
return addDays(convert!(hnsecs,
days)(duration.total!hnsecs));
else static if(is(Unqual!D == TickDuration))
return addDays(convert!(hnsecs, days)(duration.hnsecs));
}
else
{
static if(is(Unqual!D == Duration))
return addDays(convert!(hnsecs,
days)(-duration.total!hnsecs));
else static if(is(Unqual!D == TickDuration))
return addDays(convert!(hnsecs, days)(-duration.hnsecs));
}
}
If you're static if'ing each statement, it's a good sign the method should be
broken up into two overloads.
BTW, what was the problem with returning by ref?
As others pointed out, repetition hurts and hinders understanding repetition
hurts and hinders understanding repetition
hurts and hinders understanding repetition hurts and hinders understanding
repetition hurts and hinders understanding
repetition hurts and hinders understanding... :)
Finally, bordering on bikeshed, weekdays' names are long and months' are short.
Having both short and long names for
weekdays and months would solve the inconsistency and satisfy everyone.
After some discussion on the runtime list, it was decided that core.time
and std.datetime should share their Duration type, so I created time.d as
a prospective core.time and moved or copied datetime's Duration and any
other types or functions that it needed to time.d and made it so that
datetime publically imports time. So, if accepted, time would become
core.time, and datetime would become std.datetime.
Are there any plans for a Calendar interface to allow user implementations that
distinguish business days and holidays?
Also, since invariants can now be marked pure, I marked some of the
invariants pure and increased the number of functions marked as pure.
Unfortunately, due to the fact that many Phobos functions still aren't
pure (to!() and format() in particular), as well as bug #5191, many
functions that should be pure, aren't. But we're closer. Also, bug #4867
continues to prevent SysTime from being able to be immutable, and a couple
of bugs regarding invariants and templates make it so that a number of
functions that should return ref this, can't.
Maybe I'm late on the 'pure' changes, but how come all the setters are pure? I
mean it modifies the (hidden) argument of
the function, so it shouldn't be, no?
--
Tomek
Re: datetime review part 2 [Update 4]
On Wednesday 10 November 2010 10:18:27 Dmitry Olshansky wrote:
OK, I looked through the docs, to me it's very solid. I fact I never
written any code that heavily uses date-time stuff, most of the time I
just converted everything to UNIX timestamp, and back to printable form,
once the processing is done.
One suggestion about docs - since SysTime and DateTime have similar API,
wouldn't it be better to merge documentation
and represent it in form of a table (like the one in std.container,
unfortunately ATM it's screwed up )?
In the same table you could also place functions specific for SysTime,
just add a remark to it.
Another one - group overloads together, consider for instance
std.algorithm. It doesn't leak all of it's template specializations for
all kinds of ranges with the same ddoc comment. You shouldn't i.e.
const pure bool_intersects_(in Interval/interval/);
Whether the given/interval/overlaps with this/interval/.
const pure bool_intersects_(in PosInfInterval!(TP)/interval/);
Whether the given/interval/overlaps with this/interval/.
...
It only could get you tired - there is no new information at all.
May I also humbly suggest you create the second table describing generic
Interval notion (with specifics fro infinite ones marked as such).
I'm not sure the best way to do all of that. std.container tries to give a
general overview of container capabilities at the beginning and then still has
all of the specific functions listed for each type still. I could try something
like that, though making that sort of change is not something that I'm likely
to
get to very quickly. It'll take a fair bit of thinking and experimentation to
come up with the best way to do that. As for specific overloads and
documentation, I'm not sure what the best way to handle that is, but it does
need to be made clear which functions work with which types. Taking the
interval
functions for instant, some of them - like intersection - work with all
interval
types regardless of the interval type that you're dealing with, while others -
like union - only work with a subset of the interval types (since things like
the union between positively and negatively infinite intervals make no sense).
What I likely need to do is try and come up with better documentation up front
which better gets across the basic groups of types (time points, intervals,
etc.) and their basic abilities while leaving the individual documentation to
still give all of the details that it needs to. I'll look into doing that
later,
but I can't say that it's a high priority at the moment. Good suggestion though.
And one more thing about the _API_ itself:
auto interval = Interval!Date(Date(2010, 9, 2), Date(2010, 9, 27));
auto func = IRange.everyDayOfWeek!Date(DayOfWeek.monday);
auto range = interval.fwdRange(func);
I'd preffer more simple and generic things, they tend to combine with
std.algorothm so nicely (It's just an idea worth researching).
IMHO also could remove a ton of code from date-time, that do not belong
here anyway.
My try at the code above would be:
auto interval = Interval!Date(Date(2010, 9, 2), Date(2010, 9, 27));
auto onlyMondays = filter!((x){ return x.dayOfWeek == DayOfWeek.monday });
auto range = onlyMondays (interval.by!days); //reuses the same
convention for add and roll, more coherent
I'm afraid that I don't really get what you're trying to do here. A range needs
to be created from an interval. It really wouldn't make sense to do it
otherwise. And when you create a range, it needs a delegate which generates the
time points for that range. So, you create an interval and call either
fwdRange() or bwdRange() on it to give it the range-generating delegate. I
happen to have provided some helper functions for generating delegates, but
there's nothing to stop you from creating your own.
Are you trying to say that the range should automatically provide _all_
possible
time points in an interval and then you specifically filter them? That's
nowhere
near as flexible, and in the case of SysTime in particular, think about how
many
time points that is. It has hecto-nanosecond (100 ns) precision. That's 10
million time points a second. It could get really inefficient to try and filter
all
of those. Also, I'm not sure that filter would work with an infinite range (I'd
have to check), which would be a huge setback. I really don't get the benefit
of
your suggestion, though I can undkerstand it if it's not entirely clear how
ranges in std.datetime are supposed to work.
Essentially, you provide a function that takes a time point and returns the
next
time point in the range. It's a generative function, successively generating
each time point in the range as you iterate. It makes it really easy to create
infinite ranges and is completely flexible as to how time points are generated,
so
you can generate essentially any kind of range over an interval of time points
as long as
Re: datetime review part 2 [Update 4]
Latest: http://is.gd/gSwDv After some discussion on the runtime list, it was decided that core.time and std.datetime should share their Duration type, so I created time.d as a prospective core.time and moved or copied datetime's Duration and any other types or functions that it needed to time.d and made it so that datetime publically imports time. So, if accepted, time would become core.time, and datetime would become std.datetime. Also, since invariants can now be marked pure, I marked some of the invariants pure and increased the number of functions marked as pure. Unfortunately, due to the fact that many Phobos functions still aren't pure (to!() and format() in particular), as well as bug #5191, many functions that should be pure, aren't. But we're closer. Also, bug #4867 continues to prevent SysTime from being able to be immutable, and a couple of bugs regarding invariants and templates make it so that a number of functions that should return ref this, can't. In any case, essentially this update is to propose a core.time implementation and have my proposed std.datetime use it. - Jonathan M Davis
Re: datetime review part 2 [Update 4]
On Tue, 09 Nov 2010 15:03:10 -0600, Jonathan M Davis jmdavisp...@gmx.com wrote: Ugh. The datetime.d file is 1.5 MB? 0_o -- Yao G.
Re: datetime review part 2 [Update 4]
On 10.11.2010 0:51, Yao G. wrote: On Tue, 09 Nov 2010 15:03:10 -0600, Jonathan M Davis jmdavisp...@gmx.com wrote: Ugh. The datetime.d file is 1.5 MB? 0_o That must be huge unittests :) -- Dmitry Olshansky
Re: datetime review part 2 [Update 4]
Yao G.: Ugh. The datetime.d file is 1.5 MB? 0_o It contains too-much-repetitive code like: assertEqual(SysTime(DateTime(2010, 8, 1, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 733_985); assertEqual(SysTime(DateTime(2010, 8, 31, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 734_015); assertEqual(SysTime(DateTime(2010, 9, 1, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 734_016); assertEqual(SysTime(DateTime(2010, 9, 30, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 734_045); assertEqual(SysTime(DateTime(2010, 10, 1, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 734_046); assertEqual(SysTime(DateTime(2010, 10, 31, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 734_076); assertEqual(SysTime(DateTime(2010, 11, 1, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 734_077); My suggestions: - Keep only 5-10% of the tests inside the datetime.d module, and move the other 90-95% in a separated datetime test module. - Use deterministic (repeatable) random data in a loop to perform some compact fuzzy testing - Use alias or string mixing to greatly reduce the amount of text contained in the tests. Tuples help a lot. All those lines may be replaced with a single loop that contains something like: assertEqual(SysTime(DateTime(z0, x1, x2, x3, x4, x5), FracSec.from!msecs(x6)).dayOfGregorianCal, x7) Where the xn data comes from an array of typecons.Tuples like: Data5(2010, 11, 1, 23, 59, 59, 999, 734_077) Bye, bearophile
Re: datetime review part 2 [Update 4]
On Tuesday, November 09, 2010 15:48:30 bearophile wrote: Yao G.: Ugh. The datetime.d file is 1.5 MB? 0_o Heavy unit testing and documenting everything adds up. The implementation isn't all that small either, but it's the unit tests and ddoc comments which really increase the size. Most of that will never end up in any user program. It contains too-much-repetitive code like: assertEqual(SysTime(DateTime(2010, 8, 1, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 733_985); assertEqual(SysTime(DateTime(2010, 8, 31, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 734_015); assertEqual(SysTime(DateTime(2010, 9, 1, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 734_016); assertEqual(SysTime(DateTime(2010, 9, 30, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 734_045); assertEqual(SysTime(DateTime(2010, 10, 1, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 734_046); assertEqual(SysTime(DateTime(2010, 10, 31, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 734_076); assertEqual(SysTime(DateTime(2010, 11, 1, 23, 59, 59), FracSec.from!msecs(999)).dayOfGregorianCal, 734_077); My suggestions: - Keep only 5-10% of the tests inside the datetime.d module, and move the other 90-95% in a separated datetime test module. - Use deterministic (repeatable) random data in a loop to perform some compact fuzzy testing - Use alias or string mixing to greatly reduce the amount of text contained in the tests. Tuples help a lot. All those lines may be replaced with a single loop that contains something like: assertEqual(SysTime(DateTime(z0, x1, x2, x3, x4, x5), FracSec.from!msecs(x6)).dayOfGregorianCal, x7) Where the xn data comes from an array of typecons.Tuples like: Data5(2010, 11, 1, 23, 59, 59, 999, 734_077) While there seem to be a lot of tests, they've turned out to be _really_ helpful and by their very nature, they _need_ to be repetitive. Testing a lot of stuff with slight differences is where errors are likely to be found. I've definitely found errors in my code where there were a lot of tests that were similar but one of them which was just a bit different from the ones next to it managed to find a bug due to whatever quirk in the logic made it a boundary of some kind in the calculation. Having all of those similar tests is valuable. Now, some of those could be turned into loops, but that would complicate the code, it would be harder to figure out what failed (since things would have the same line number - though using assertEqual() instead of assert would certainly help), and many of the tests are different enough that it wouldn't work anyway. In fact, while a lot of the tests are similar, many of them aren't in a pattern that would turn into a loop very well. Also, the unit testing model in D and how Phobos is set up isn't really intended to have the unit tests separate from the code. Doing so would be cumbersome and more bug prone. It's irritating enough to have to put the tests for some of the templated types after the type definition rather than in it. Managing the tests is _much_ easier when they're right next to the functions that they're testing. On top of that, many of them need private access, which they can't get in another module. So, while some could be moved out, I don't think that it would be a good idea, and many couldn't be moved out even if you wanted to. I grant you that there are a lot of unit tests, and ways probably could be found to trim them down, but they've helped a _lot_ in ensuring the code is correct, they have no effect whatsoever on user code since they'll be compiled-out, and I have found that maintaining them next to the functions that they're testing is actually easier to manage that it would be to try and split them out. So, I'm not particularly inclined to drastically change my tests. I think that the real question here is how good the API is. - Jonathan M Davis
Re: datetime review part 2 [Update 4]
On Tue, 09 Nov 2010 18:11:59 -0600, Jonathan M Davis jmdavisp...@gmx.com wrote: I think that the real question here is how good the API is. I think that the sheer size of the library, specially with a datetime.d file with almost 31,600 lines of code (granted, most of them are unit test and documentation), makes a little difficult to analyze or give a proper review of the code. It will takes a fair share time to do it. Maybe that's why very few people has given criticism or suggestions. I only gave a cursory view (you need to scroll a lot just to find the implementation code), but I'll give it a more throughly review tonight. -- Yao G.
Re: datetime review part 2 [Update 4]
Jonathan M Davis:
While there seem to be a lot of tests, they've turned out to be _really_
helpful
and by their very nature, they _need_ to be repetitive.
Repetitive doesn't mean it has to take lot of space. Tests are a cross between
code and documentation, and both are better not too much redundant.
Now, some of those could be turned into loops, but that would complicate the
code, it would be harder to figure out what failed
I am not sure of this.
Also, the unit testing model in D and how Phobos is set up isn't really
intended
to have the unit tests separate from the code.
Then maybe the nature of unittesting in D needs an improvement :-)
(From what I have seen in the last few years, in the process of D design often
up-front design is furiously refused. Post-problem-patch-driven design is much
more appreciated in this language. So if problems are found in practical
unittesting in D, then maybe designers may take in consideration the
possibility of improving the situation).
On top of that, many of them need private access, which they can't get in
another module.
For some people this is a quite controversial point.
An option may be to modify the compiler so it allows to override protection in
code contained inside unittest{...}.
I grant you that there are a lot of unit tests, and ways probably could be
found
to trim them down,
Just to avoid misunderstandings: deleting unit tests is the thing I have never
suggested.
Bye,
bearophile
Re: datetime review part 2 [Update 4]
An option may be to modify the compiler so it allows to override protection
in code contained inside unittest{...}.
Another option is to modify the compiler so it sees unittests modules as part
of another module (using a mixin() on a string import is too much hacky).
Bye,
bearophile
Re: datetime review part 2 [Update 4]
On Tuesday, November 09, 2010 16:23:56 Yao G. wrote: On Tue, 09 Nov 2010 18:11:59 -0600, Jonathan M Davis jmdavisp...@gmx.com wrote: I think that the real question here is how good the API is. I think that the sheer size of the library, specially with a datetime.d file with almost 31,600 lines of code (granted, most of them are unit test and documentation), makes a little difficult to analyze or give a proper review of the code. It will takes a fair share time to do it. Maybe that's why very few people has given criticism or suggestions. I only gave a cursory view (you need to scroll a lot just to find the implementation code), but I'll give it a more throughly review tonight. That's quite understandable, but that's part of the reason that the ddoc html files are there. You don't actually have to go trolling through the code to look at the API. - Jonathan M Davis
Re: datetime review part 2 [Update 4]
On Tuesday, November 09, 2010 16:42:32 bearophile wrote:
An option may be to modify the compiler so it allows to override
protection in code contained inside unittest{...}.
Another option is to modify the compiler so it sees unittests modules as
part of another module (using a mixin() on a string import is too much
hacky).
Having named unit tests would help a lot. Without those splitting out the unit
tests is a total no-go for me. I have definitely found that managing the unit
tests is easier when they are next to the functions that they're testing rather
than elsewhere. If they were named, then they would be easier to keep track of
at it would not be as big a deal, but it would stilll be problematic since it
would be harder to verify that each function is indeed properly tested. As it
is, you can just look right under the function to see its unit tests. It does
make it harder to find the actual functions, but I think that the
maintainability
of having the unit tests right next to the functions is worth more. A proper
IDE
would even negate the problem entirely, since then you could click on the
function name to go to.
As for simplifying the tests, a lot of them just would not turn into a loop
very
well unless you were willing to increase the number of values being tested by a
fair margin. That would make the tests take that much longer to run. I'm sure
that if I took the time to, I could reduce some of the tests in terms of lines
of code without reducing what is being tested, but that's a lot of work to go
to
at this point, and I have better things to be doing. Having the number of tests
that I do has been a big help, and if anything, there probably aren't enough of
them, so I'm not going to listen to any arguments to reduce their number (which
you apparently weren't arguing). I do agree that reducing their space could be
useful, but again, it would be a lot of work to do - many don't reduce as
nicely
as they might first appear to, and that's a lot of tests to got through. If
Andrei were to insist that it be done before being put into Phobos, then I'd
have to do it, but as it is, they work, and I'd rather be spending my time on
more productive things. Maybe I'll spend some time to try and reduce the test
line count without reducing the tests at some point, but it does not seem worth
it right now.
- Jonathan M Davis
Re: datetime review part 2 [Update 4]
On Tue, 09 Nov 2010 19:34:15 -0600, Jonathan M Davis jmdavisp...@gmx.com wrote: That's quite understandable, but that's part of the reason that the ddoc html files are there. Yes, but I also wanted to look at the implementation. You don't actually have to go trolling through the code to look at the API. I see what you did there :) -- Yao G.
Re: datetime review part 2 [Update 4]
On Tuesday, November 09, 2010 17:34:15 Jonathan M Davis wrote: On Tuesday, November 09, 2010 16:23:56 Yao G. wrote: On Tue, 09 Nov 2010 18:11:59 -0600, Jonathan M Davis jmdavisp...@gmx.com wrote: I think that the real question here is how good the API is. I think that the sheer size of the library, specially with a datetime.d file with almost 31,600 lines of code (granted, most of them are unit test and documentation), makes a little difficult to analyze or give a proper review of the code. It will takes a fair share time to do it. Maybe that's why very few people has given criticism or suggestions. I only gave a cursory view (you need to scroll a lot just to find the implementation code), but I'll give it a more throughly review tonight. That's quite understandable, but that's part of the reason that the ddoc html files are there. You don't actually have to go trolling through the code to look at the API. I should probably add that it would be worth it for folks to just look at the documentation for what they would generally try and do with a datetime module and see whether they can do it reasonably well and what problems they'd have. While looking at the module as a whole is definitely needed, just having folks look at the portions that they'd use and commenting on that would be useful. Is getting the time easy enough and intuitive enough? Is doing what you need to do with time in basic applications once you have it easy or intuitive enough? Etc. - Jonathan M Davis
Re: datetime review part 2 [Update 4]
On Tuesday 09 November 2010 17:45:51 Yao G. wrote: On Tue, 09 Nov 2010 19:34:15 -0600, Jonathan M Davis jmdavisp...@gmx.com wrote: That's quite understandable, but that's part of the reason that the ddoc html files are there. Yes, but I also wanted to look at the implementation. You don't actually have to go trolling through the code to look at the API. I see what you did there :) ??? Oh my. Ouch. I did not mean trolling. I meant _trawling_. LOL. Sorry about that. That was _not_ on purpose. Funny though. Feel free to look through the implementation. I just tried to make it so that it was reasonably easy to look at the API without worrying about the implementation. And while I do think that the implementation is important, I think that the API is far more important. To a great extent, the implementation can be fixed later if it needs to be, while the API can't really afford to change much once it's in Phobos. So, review it however you'd like, but you don't _have_ to look at the implementation, I do believe that the API itself is what's most critical. - Jonathan M Davis
