Closed Geod24 closed 4 years ago
Fixed the test failure and reduced the diff
Just to get a sense where this is going: is the idea here to preserve the public TimeSpan
APIs, but to allow all private ocean functionality to be rewritten using Duration
? Then add public Duration
APIs and deprecate TimeSpan
and public APIs that use it?
Done:
ticks
to mention hnsecs;impl
to duration_
;Just to get a sense where this is going [...]
Yes, that's it. I was looking at this package and the amount of duplication / unused modules. This was a low hanging fruit. There are other places that I was looking at, but given that Ocean won't be used by its creators anymore, I am not sure how worth it is.
FYI this is our current diff: https://github.com/bpfkorea/ocean/commit/b50157ea3025b9d354b85ac33d8c2975c7e6cb17
Needs release notes (this change should be highlighted, since even though it should theoretically be non-breaking, it's possible there will be some unanticipated behavioural change).
Have you benchmarked just in case there are some performance impacts? I wouldn't see that as a blocker but it would be good to know. (No worries if not.)
Other than that, and the minor remarks above, LGTM.
Needs release notes
We've never done release notes for refactoring. I plan to deprecate it in the future, that's where the release notes should be.
Have you benchmarked [...]
No
We've never done release notes for refactoring.
It's not mere refactoring (this patch adds a new constructor), and in any case, if an implementation has changed significantly enough that it could have downstream impact, it's only polite to alert users to that possibility.
The alias this
and duration
method are also new. So, this genuinely does change what the data structure can do, and what users can do with it. It's not just under the hood changes.
Ping @don-clugston-sociomantic
Honestly I don't see any need to be pedantic about release notes, since very few people are affected. I'll give the two of you one day more to battle it out, and then merge it. FYI: I have 8 more days with commit rights to this repo, I doubt there will be any more activity on it after that. So I really don't want perfection to get in the way of good enough. I'd love to see the end of hnsecs, which seems to not exist anywhere in the world outside of Phobos, and which is clearly somewhat platform-specific.
Well, I've given my suggestions. No big deal if there is disagreement ;-)
Ping @don-clugston-sociomantic
LGTM
This reimplements TimeSpan as a small wrapper around
core.time : Duration
. The intent is to ultimately replaceTimeSpan
withDuration
.This will have multiple advantages:
Thread.sleep
);Like the old utilities in Phobos, the
time
package of Ocean is built around ticks, with 1 tick equivalent to 1 hnsecs, or 100 nano seconds.However, Phobos moved away from using ticks directly, as they do not accurately represent time: for example, some API acception time objects might expect monotonic times, while other might expect "user time" (leap second and other non monotonic behavior).
In order to adapt Ocean to this distinction, the first step is to reduce the amount of duplication and special casing, of which removing
TimeSpan
contribute. This does not actually deprecateTimeSpan
yet (as many places still rely on it), but allow to start replacing it.