Skip to content
This repository has been archived by the owner on Jul 19, 2024. It is now read-only.

Aligned the "Minute" members of intraday and historical data points. #103

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

karagog
Copy link
Contributor

@karagog karagog commented Dec 12, 2021

They appear to both be used the same way, as a string field that tells
us the minute of the data point in the time zone of the exchange.

I also removed a helper function that attempts to get the time from the
historical data point by combining the date and minute fields. The
problem is that this results in the wrong time because the time objects
are in UTC, so to do this right you'd need to know the time zone which
the "minute" value is in. I think we should leave it up to the consumer
to write their methods to convert times according to their special
circumstances.

@karagog
Copy link
Contributor Author

karagog commented Dec 12, 2021

TL;DR is that I changed HistoricalDataPoint.Minute from a string to an HourMinute object, the same way as IntradayPrice.

@karagog karagog force-pushed the historical branch 3 times, most recently from ad436ff to 2947add Compare February 17, 2022 07:51
They appear to both be used the same way, as a string field that tells
us the minute of the data point in the time zone of the exchange.

I also removed, rather than update, a helper function that attempts to get the time from the
historical data point by combining the date and minute fields. The
problem is that this results in the wrong time because the time objects
are in UTC, so to do this right you'd need to know the time zone which
the "minute" value is in. I think we should leave it up to the consumer
to write their methods to convert times according to their special
circumstances.
@karagog
Copy link
Contributor Author

karagog commented Feb 17, 2022

FYI this merges cleanly again after the updates to main.

@matthewrankin
Copy link
Contributor

Do you think this is a breaking change that should be delayed until a major version bump to maintain semantic versioning?

@karagog
Copy link
Contributor Author

karagog commented Mar 22, 2022

Yes this is potentially a breaking change, if someone is depending on the Time() method of historical data point (although I'm not sure how someone even uses that correctly, due to the lack of time zone). Otherwise I think parsing the json response is backwards compatible.

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

Successfully merging this pull request may close these issues.

None yet

2 participants