Fix deprecation warnings in std.datetime.

[jmdavis]

With the deprecation of the individual unit getters on Duration, these lines need to be changed in
std.datetime to get rid of the deprecation warnings.

https://issues.dlang.org/show_bug.cgi?id=12846

[brad-anderson]

LGTM. If you do end up changing getOnly based on the other pull request's discussion I'd just include this change in the same pull request that makes that change.

[jmdavis]

LGTM. If you do end up changing getOnly based on the other pull request's discussion I'd just include this change in the same pull request that makes that change.

Except that you can't do that, because that would be in core.time, which is in druntime. We'd either have to temporarily add an alias for getOnly when renaming it to whatever the new name was, and then remove that in another pull after making the changes to std.datetime, or we'd have to just do both pull requests at once, meaning that there would be breakage if someone wasn't careful enough and merged only one of the two.

[brad-anderson]

Oh, right. That split implementation between phobos and druntime has always felt odd but I can see the reasons for it. Just thinking aloud a bit, have you ever thought of perhaps ripping out core.time from druntime and placing it phobos then providing some sort of minimal type in druntime that Duration could implicitly convert to (CoreHnsecs or something) that would be used in the duration taking functions of druntime (Thread.sleep, et.al).

In theory nothing would break and this would allow us to make std.datetime even bigger than it already is which should be an overriding goal. 😐

[jmdavis]

Just thinking aloud a bit, have you ever thought of perhaps ripping out core.time from druntime and placing it phobos then providing some sort of minimal type in druntime that Duration could implicitly convert to (CoreHnsecs or something) that would be used in the duration taking functions of druntime (Thread.sleep, et.al).

I've never thought about anything like that, and I don't see why it would be particular beneficial. On the whole, the fact that Duration is in core.time rather than std.datetime isn't a problem, and the only reason that we'd have a merge problem in this case is because we'd be renaming a function shortly after it was introduced into druntime and after Phobos was already using it. And I'd hate to add yet more confusion by introducing more Duration types. As it is, I intend to deprecate TickDuration after I have a new type to represent the monotonic time ready, as TickDuration is causing all kinds of confusion.

[schveiguy]

I don't think this pull should be done unless we can resolve the issues being discussed on druntime pull 822. I disagree with the deprecation of API so quickly, normally it is a longer process.

Please revert the druntime pull, so we are not impeding development, and then we can discuss further the API changes.

[jmdavis]

I disagree with the deprecation of API so quickly, normally it is a longer process.

In what respect? Are you talking about marking something as "scheduled for deprecation"? We did before, because deprecating something generated errors, not warnings, and because there were no messages with deprecations. Now, deprecating something generates warnings with nice messages, so if we're going to deprecate something, I see little point in marking it as "scheduled for deprecation" first. We can just mark it as deprecated immediately and simply leave it as deprecated for longer before we make it undocumented and then finally remove it.

[brad-anderson]

I've never thought about anything like that, and I don't see why it would be particular beneficial.

I was thinking more of moving core.time into Phobos for the end users, not really for the developers. Have it all in one place is more logical and intuitive. Not terribly important though, I was just thinking aloud of ways it could be done.

Now, deprecating something generates warnings with nice messages, so if we're going to deprecate something, I see little point in marking it as "scheduled for deprecation" first.

And good riddance. "Scheduled for deprecation" was such a terrible anti-pattern which only existed because of fear of warnings being ignored. Having stages of deprecation never made sense. Something should either be deprecated or not deprecated. It shouldn't be stuck in some sort of deprecation limbo.

(Sorry, had to rant. I always hated scheduled deprecation)

[jmdavis]

And good riddance. "Scheduled for deprecation" was such a terrible anti-pattern which only existed because of fear of warnings being ignored.

No, it existed because it used to be that deprecating something generated an error and thus immediately broke code. So, we needed to warn people first so that they had a chance to change their code before it broke. Now, deprecating something generates a warning rather than an error, so deprecations no longer immediately break code (and the warning helpfully tells you exactly where you're using the deprecated function, whereas "Scheduled for deprecation" didn't, since it was just documentation). So, while I agree that we don't need "Scheduled for deprecation" any longer, it did serve a purpose before.

[schveiguy]

Are you talking about marking something as "scheduled for deprecation"? We did before, because deprecating something generated errors, not warnings, and because there were no messages with deprecations.

I honestly did not know this was changed! I expected that the deprecation would cause code not to compile unless you passed a certain switch (was it -deprecated?). But what about those that compile with warnings as errors?

[brad-anderson]

By "fear of warnings being ignored" I meant the general attitude about not using warnings in DMD because they may be ignored which is why using deprecated symbols was an error in the first place. It did serve a purpose but only because the original decision to make it an error was a bad one.

[schveiguy]

Just thinking aloud a bit, have you ever thought of perhaps ripping out core.time from druntime and placing it phobos

The reason we have core.time is because so many OS primitives rely on durations, e.g. timeval_t and timespec_t. Putting yet another time type in phobos that duplicates the functionality of Duration, I don't see the point in it. We can easily include the documentation for Duration in std.datetime's DDoc.

[jmdavis]

I honestly did not know this was changed! I expected that the deprecation would cause code not to compile unless you passed a certain switch (was it -deprecated?). But what about those that compile with warnings as errors.

I fought for the change a while ago, and one of the compiler devs was kind enough to implement it. Without it, we couldn't really deprecate anything (especially as Walter has become more paranoid about breaking changes). That combined with having deprecation messages actually makes the process somewhat pleasant now. So, while we should still be smart about what we deprecate (and should strive for not needing to deprecate anything), we at least can do it without breaking code now.

-d will still turn off the deprecation messages but is arguably pretty stupid to use at this point, and -w has no effect the deprecation messages - they're not treated as normal warnings.

[schveiguy]

OK, good to know. So it's purely an argument on the merits and not the breakage of code. I still think the other pull should be discussed further. You can understand why I was a little alarmed that it was merged so quickly now :)

[jmdavis]

OK, good to know. So it's purely an argument on the merits and not the breakage of code. I still think the other pull should be discussed further.

I have pull sitting on auto-merge which reverts the changes and will open a new pull with the changes again (and getOnly renamed) this evening.

You can understand why I was a little alarmed that it was merged so quickly now :)

Yeah, your reaction seemed a bit extreme, but it's certainly understandable now

However, I do feel strongly that the changes should be made. Everyone I've worked with who has used Duration has screwed up and used get when they meant total at least once, and it's caused us some subtle, nasty bugs. I'm convinced that we need a name that's less bug-prone, and based on previous discussions, the individual getters are a frequent source of confusion, causing similar issues.

[brad-anderson]

I, too, can speak from experience because Boost Date Time uses seconds(), minutes(), etc. for what get() does and has total_seconds(), total_minutes() for the total time. It took awhile and a few bugs to finally get it in my head what seconds() et. al. did (which is something I almost never needed).