Skip to content
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

start and stop shouldn't be defined for TimePeriod #40

Open
ericphanson opened this issue Jun 30, 2022 · 2 comments
Open

start and stop shouldn't be defined for TimePeriod #40

ericphanson opened this issue Jun 30, 2022 · 2 comments
Labels

Comments

@ericphanson
Copy link
Member

"""
start(span)
Return the inclusive lower bound of `span` as a `Nanosecond` value.
"""
start(span::TimeSpan) = span.start
start(t::Period) = convert(Nanosecond, t)
"""
stop(span)
Return the exclusive upper bound of `span` as a `Nanosecond` value.
"""
stop(span::TimeSpan) = span.stop
stop(t::Period) = convert(Nanosecond, t) + Nanosecond(1)

I get that it's so that TimePeriod's act like a 1-ns range around that time, but I think it's overly error prone. E.g.

duration(time_from_index(fs, size(data, 2)))

gives 1 Nanosecond always, where the intended code is

 duration(time_from_index(fs, 1:size(data, 2)))

IMO in this case a clear error from the first example would be preferable.

@ararslan
Copy link
Member

ararslan commented Jul 5, 2022

There is a potential alternate behavior of the TimeSpans interface for Periods that I think could arguably make more sense: start(period) is 0 and duration(period) == period. I have on at least one occasion forgotten about the one-nanosecond behavior and thought it worked this way. I think it would actually make your two time_from_index examples behave identically.

@ericphanson
Copy link
Member Author

I think that kinda makes sense, but the ambiguity seems like a good argument that these should be MethodError's instead, so you have to specify

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants