[phobos] Split std.datetime in two?

[Jonathan M Davis]

On Thursday, February 10, 2011 14:28:22 Andrei Alexandrescu wrote:
> On 2/10/11 11:33 AM, Jonathan M Davis wrote:
> > Well, as Steven says, most of std.datetime is unit tests. And probably
> > over half of the rest is documentation, since most of the functions
> > themselves are relatively short and simple.
> 
> That means there are too many unit tests. There is such a thing as too
> much of a good thing.
> 
> > Add to that that I tend to like a fair bit of
> > vertical space in my  code (e.g. return statements pretty much always
> > have a blank line above them), and how I code tends to take up a fair
> > bit more LOC than you would without actually being more code.
> 
> As I mentioned, it would be courteous to your other colleagues if you
> were more economical with vertical space. Many people (and most of
> Phobos devs) consider vertical space a rare commodity.
> 
> > It wouldn't surprise me that if I
> > were to format std.algorithm in a manner that I'd typicall write it, it
> > would at least double in line count simply due to additional blank lines
> > that I'd introduce for code legibility.
> 
> I consider legible code compact code that I can keep on one screen and
> understand what it does.

There aren't very many functions in std.datetime which wouldn't fit one one 
screen. Regardless, it's obvious that code legibility is a subjective thing.

> > And, of course, if I'd written std.algorithm, it
> > would almost certainly have had quite a few more unit tests. None of that
> > increases the size of the actual binary.
> 
> It's not the size of the binary I'm worried about.
> 
> > I don't think that std.datetime is the
> > largest module in Phobos, but I do think that comparing its line count to
> > something like std.algorithm really tells you how much of a difference it
> > makes in the size of the binary - especially when so much of it is unit
> > tests which wouldn't end up in a binary.
> 
> One thing is certain: someone who wants to take a look at std.datetime
> has the prospect of navigating 34KLOC.
> 
> > The unit testing functions I wrote are pretty much in the same boat. They
> > had thorough unit tests - testing not only all of its code paths (which
> > is what the % code coverage gives you) but an appropriate range of
> > values. The functions themselves weren't all that long.
> 
> They are unrolled loops that should be rolled. Most likely they test the
> same thing several times.

Some of them could be easily turned into loops - others not (most probably not). 
They don't generally quite test a set of consecutive values, though there is 
often a pattern to them. I tend to dislike doing much complicated just to try 
and reduce the lines in a unit test, because the risk of screwing it up goes up, 
and if the tests are wrong, the code will be wrong. So, the tests are simple but 
repetitive.

The risk of screwing up like that for std.datetime is lower at this point, since 
it has already been thoroughly tested (the likelihood of a test failure when 
changing the tests is now much more likely to be a screwed up test rather than 
the code being broken). So, I'd be far more willing to try to alter the tests to 
take up less space now that I would have been when writing it.

I suppose that I could look at trying to turn some of the tests into arrays 
which get looped over. That sort of thing is not always going to be possible 
though, regardless of how repetitive the tests may seem. And while the functions 
aren't generally long, and _most_ of them are simple, there _are_ some of them 
with calculations which are easy to screw up, and a lot of tests are necessary 
to catch edge cases and the like. For instance, at one point, I thought that I'd 
finally gotten the gregorian day calculations right only to find that when I added 
tests for negative years which ended in 99, the result was always 1 day off. I 
only caught it because I had a lot of tests.

While I'm fine with the tests as they are, obviously others aren't. So, I'll see 
if I can reasonably reduce how many lines they take up. But I'm not particularly 
willing to reduce what's being tested.

> > P.S. Weren't you considering increasing the character limit beyond 80?
> > I'm not about to go and edit std.datetime for line width until that's
> > definitely decided. At the moment, I'm working on improving the
> > documentation (mostly for consistency rather than length, though its
> > length should be somewhat reduced when I'm done) and that will make the
> > documentation and examples at most 80 characters wide, but I haven't
> > started in on the rest of it yet. Most of the long lines are unit tests
> > though, I believe (some of which are ludicrously long simply because
> > it's easier to read an assertion on a single line).
> 
> Generally I don't want to impose something as much as us all reaching a
> consensus about it. Some of Phobos people (I recall you and Don) prefer
> larger limits. On the other hand, there _are_ some things that will
> never reach an agreement so someone needs to just set some limits.

We need a decision or consesus of some kind. I really hate the 80 character 
limit, but we need to decide on a limit of some kind - be it 80, 100, 120 or 
whatever. Once we've decided, I can make std.datetime match it.

It was discussed on the D list, but no decision was actually ever made from what 
I can tell.  Sean, Don, and I all wanted longer line counts (100+). Other folks 
wanted 80. However, most of the folks who chimed in on that thread aren't Phobos 
developers, for however much that matters (in fact, other than you, I think that 
Steven might have been the only other Phobos developer to chime in - unless you 
count Walter's vote of 1 mol). Going off of just the Phobos developers in that 
thread, the consensus would be something around 100, but that was definitely not 
all of them.

I'd love it if the limit were something more like 100 than 80, but at this 
point, I think that a decision just needs to be made.

- Jonathan M Davis