-
-
Notifications
You must be signed in to change notification settings - Fork 746
Simplified std.datetime.package overview #6356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for your pull request, @JackStouffer! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + phobos#6356" |
|
This is just another step along the way to making std.datetime more accessible. I also want to add top level examples to all datetime modules to quickly introduce the concepts of the module. |
ba690ab to
aa9df33
Compare
wilzbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I never liked this huge wall of text.
Waiting for @jmdavis's approval though.
| import std.datetime.timezone : UTC; | ||
| import core.time : days; | ||
|
|
||
| auto st = SysTime(DateTime(2018, 1, 1, 12, 30, 10), UTC()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use an actual TimeZone here, s.t. it's non-trivial?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UTC is a TimeZone.
|
Haha ironically auto-tester is running in a spurious failure in std.datetime in this PR on FreeBSD: https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=3094297&isPull=true |
aa9df33 to
6901b47
Compare
|
What's up with MREF throwing out malformed HTML? |
std/datetime/package.d
Outdated
| This functionality is separated into the following modules | ||
| $(UL | ||
| $(LI $(MREF std, datetime, date)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to use _datetime here. While opt-out of automatic keyword highlighting is available since 2.079, it hasn't been applied to dlang.org yet:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's turn the switch -> dlang/dlang.org#2307
58d4f7d to
905a6b9
Compare
jmdavis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. I can't say that I particularly like these changes. Turning some of the information into a table is definitely an improvement, but a lot of information is lost here. It does need to be rewritten on some level, with some of it being in the specific modules and some of it being here, but throwing it all out seems like a terrible idea to me. However, I can't really direct anyone on how to rewrite it. I'm just going to have to find the time to do it myself. Maybe it's better to let these changes through (with a few tweaks) and have the information here be butchered until I'm able to improve it, but I don't know.
| $(LREF StopWatch)) | ||
| $(LI Benchmarking functions.) | ||
| $(LI Various helper functions.) | ||
| Phobos provides the following functionality for time: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dates and times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is referring to time the concept and not time on the clock.
std/datetime/package.d
Outdated
| $(TR | ||
| $(TD Intervals of Time) | ||
| $(TD | ||
| $(REF_ALTTEXT Interval, Interval, std, _datetime, interval)$(NBSP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're trying to list the various types, even if you don't care about the range types in the module, there is also PosInfInterval and NegInfInterval.
| import std.datetime.timezone : UTC; | ||
| import core.time : days; | ||
|
|
||
| auto st = SysTime(DateTime(2018, 1, 1, 12, 30, 10), UTC()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UTC is a TimeZone.
|
|
||
| auto st = SysTime(DateTime(2018, 1, 1, 12, 30, 10), UTC()); | ||
| assert(st.toISOExtString() == "2018-01-01T12:30:10Z"); | ||
| st += 2.days; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confess that I hate this syntax and would much prefer days(2), though some folks do seem to really like that syntax.
| ) | ||
| ) | ||
| $(TR | ||
| $(TD Durations of Time) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about these. They're all in core.time, not std.datetime, though they're used quite heavily in std.datetime. So, on the one hand, it's kind of odd to list things, and on the other hand, it does make some sense. If they're going to be listed though, then there should also be a section for the monotonic time listing core.time.MonoTime.
|
I don't think much information is lost. A lot of what's written here is covered in the documentation for the modules and for the types. |
905a6b9 to
0758786
Compare
The only thing that leaps out to me as needing module-level explanation is a table of accepted time unit strings. (For ease of learning units probably should have been an enumerated type rather than raw strings, but documentation is for what exists.) |
0758786 to
b814453
Compare
I think that would go on the individual module docs. @jmdavis Ping. |
std/datetime/package.d
Outdated
| $(TR | ||
| $(TD Points in Time) | ||
| $(TD | ||
| $(REF_ALTTEXT Date, Date, std, _datetime, date)$(NBSP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preventing auto-highlighting is no longer needed :)
see e.g. dlang/dlang.org#2307
std/datetime/package.d
Outdated
| $(LI $(MREF std, _datetime, timezone)) | ||
| $(LI $(MREF std, _datetime, systime)) | ||
| $(LI $(MREF std, _datetime, interval)) | ||
| $(LI $(MREF std, _datetime, stopwatch)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a short, one-line description of each of these packages, s.t. the user knows what the difference betwen date and systime is?
Some of the information that was here should be in the package documentation as part of an overview, and some if really just belongs in the modules. The result of these changes is that we pretty much just end up with a table listing the types and a distinct lack of any kind of explanation. I guess that I'll let Seb merge this when his requested changes are done, but pretty the only way that I'm going to be happy with this is if I take the time to go over the module documentation for each module in std.datetime (including package.d) and rework it. In the interim, at least these changes get rid of the reference to std.datetime being a module, and the table cleans up what information is kept even if just about everything else is lost in the process. Unfortunately, I simply don't have the time to spend on reworking the docs right now. I guess that I'll try to tackle that during dconf. |
b814453 to
a737f62
Compare
Many people have remarked that
std.datetimeis confusing: https://github.com/wilzbach/state-of-d-2018/blob/master/12h:%20Which%20parts%20of%20Phobos%20(D%27s%20Standard%20Library)%2C%20if%20any%2C%20do%20you%20find%20difficult%20to%20work%20with%3FThis is the first step to improving that situation.
This PR: