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

More generic NamedTuple handling #42

Open
haberdashPI opened this issue Jul 12, 2022 · 4 comments · May be fixed by #41
Open

More generic NamedTuple handling #42

haberdashPI opened this issue Jul 12, 2022 · 4 comments · May be fixed by #41

Comments

@haberdashPI
Copy link
Member

As noted in #41 by @ararslan:

  • It should be possible to support arbitrary NamedTuples that have :start and :stop at minimum, possibly as part of a larger set of keys, e.g. (; start=Second(1), stop=Second(5), event="explosion"). AFAICT the only real tradeoff is that you get an ErrorException rather than a MethodError if you're missing :start or :stop.
  • IMO, the :start and :stop keys needn't be Nanosecond-valued or even Period-valued; in the same manner that e.g. TimeSpan(0, 10) works, we could convert to Nanosecond as needed by defining
    start(nt::NamedTuple) = Nanosecond(nt.start)
    stop(nt::NamedTuple) = Nanosecond(nt.stop)
    which should provide the same set of guarantees.
  • ...actually, we could even define an untyped fallback method that does exactly the above, which would cover NamedTuples for free. Hmmm.
@haberdashPI haberdashPI linked a pull request Jul 12, 2022 that will close this issue
@kleinschmidt
Copy link
Member

I think automatically supporting anything that can be converted to Nanosecond is a huge footgun. It's not hard to imagine someone creating a timespan with e.g. Ints giving start/stop in seconds or milliseconds without fully understanding what's going on internally and having mysterious, silent breakage (when comparing to other spans with nanosecond-based times).

@kleinschmidt
Copy link
Member

(of course, the same goes for the TimeSpan(0, 10) constructor)

@haberdashPI
Copy link
Member Author

Oh good point... mm... for now maybe the #41 should only accept tuples with Period values.

@haberdashPI haberdashPI linked a pull request Jul 12, 2022 that will close this issue
@ararslan
Copy link
Member

Yeah, that's a very good point. Scratch that part then.

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

Successfully merging a pull request may close this issue.

3 participants