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

Fix types inconsistency on API level with time #470

Closed
timocov opened this issue Jun 8, 2020 · 0 comments · Fixed by #934
Closed

Fix types inconsistency on API level with time #470

timocov opened this issue Jun 8, 2020 · 0 comments · Fixed by #934
Assignees
Labels
breaking change Changes the API in a non backwards compatible way. enhancement Feature requests, and general improvements.
Milestone

Comments

@timocov
Copy link
Contributor

timocov commented Jun 8, 2020

Currently we have several "form" of time:

  • inbound time - it might be "business day string", "business day object" and "utc timestamp"
  • outbound time - even if we declared it as Time (what means the same as "inbound time"), but actually it might be "business day object" and "utc timestamp" only, because we strip strings on API level and convert them in "business day object"
  • "time comes from model level" - that times, which are generated on model level, which knows nothing about Time and knows about "business day object" and "utc timestamp" (the same as outbound one, but more explicit on type level)

I think we need to make it more consistency and leave the only one possible time value (whatever it will be) (or maybe two of them, but split them out and make somehow).

In internal discussion @eugene-korobko suggests to use "inbound time" everywhere, and provide the same time value an user passed to the library (currently we parse/convert it and just forget about the original value). But there is question "what we need to do if we have 2 series, one has data with "business day object" and another one with "business day string", and they have the same time point, what value we should provide here? Which was set first?

Me personally thinks that it might be better to split explicitly "inbound" and "outbound" types and use them (actually it's pretty the same we currently have, but without having wrong types somewhere).

@timocov timocov added need more feedback Requires more feedback. needs investigation Needs further investigation. labels Jun 8, 2020
@timocov timocov added this to the 3.1 milestone Jun 8, 2020
@timocov timocov modified the milestones: 3.1, Future Jun 23, 2020
@timocov timocov modified the milestones: Future, 4.0 Nov 13, 2020
@timocov timocov modified the milestones: 4.0, Future Jun 14, 2021
@timocov timocov modified the milestones: Future, 4.0 Nov 2, 2021
@timocov timocov added breaking change Changes the API in a non backwards compatible way. enhancement Feature requests, and general improvements. and removed need more feedback Requires more feedback. needs investigation Needs further investigation. labels Nov 2, 2021
@timocov timocov linked a pull request Dec 15, 2021 that will close this issue
3 tasks
@timocov timocov closed this as completed Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes the API in a non backwards compatible way. enhancement Feature requests, and general improvements.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants