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.

Sign up for GitHub

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

Jump to bottom

Added get_ticker_history and tests to client library. #319

Merged
merged 3 commits into from
Nov 28, 2022
Merged

Added get_ticker_history and tests to client library. #319

merged 3 commits into from
Nov 28, 2022

Conversation

antdjohns
Copy link
Contributor

No description provided.

@antdjohns antdjohns marked this pull request as ready for review October 27, 2022 02:34
@antdjohns antdjohns requested a review from jbonzo October 27, 2022 02:44
jbonzo
jbonzo reviewed Oct 27, 2022
View reviewed changes
Copy link
Collaborator

@jbonzo jbonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I am curious why we use the phrase "events" when the endpoint says "history" maybe this should lead to a broader discussion

@antdjohns
Copy link
Contributor Author

I think I got caught up on “events” vs history because history is an all encompassing thing where as events has a connotation of specificity.

the history endpoint sends back ticker_change events not history; but, events are historical… in the future will the history endpoint return all event types?

I can definitely go back and fix the “semantic discrepancies”/incorrect naming.

@antdjohns
Copy link
Contributor Author

Also, events is sexier than history.
events or history.
history <- old and boring.
events <- boom.
As a user I would feel “cooler” hitting and api for events rather than history.

@antdjohns antdjohns changed the title Added get_events and tests to client library. Added get_ticker_history and tests to client library. Oct 27, 2022
@antdjohns antdjohns requested a review from jbonzo October 27, 2022 13:47
@jbonzo
Copy link
Collaborator

jbonzo commented Oct 27, 2022

Also, events is sexier than history. events or history. history <- old and boring. events <- boom. As a user I would feel “cooler” hitting and api for events rather than history.

Ha I like the marketing here. My main thinking is the docs should be in complete parity with our client libraries. Currently our docs have the history wording
Uploading image.png…

Let's bring this up in the next milestone for feedback from others.

@danielatpolygonio
Copy link
Contributor

danielatpolygonio commented Oct 27, 2022 •
edited
Loading

I think I agree with events now that the API has changed in scope, and might in the future include events that haven't happened yet (scheduled ticker changes, acquisitions, etc) -https://polygonio.slack.com/archives/C045MDVRB5M/p1666812291441019?thread_ts=1666807588.787899&cid=C045MDVRB5M

Should we just ask in the channel or wait until the milestone? This isn't a blocker so no rush, but if we can get the answer before the meeting we can go ahead and change the meeting and avoid a bit of derailing perhaps

@jbonzo
Copy link
Collaborator

jbonzo commented Oct 27, 2022

I think I agree with events now that the API has changed in scope, and might in the future include events that haven't happened yet (scheduled ticker changes, acquisitions, etc) -https://polygonio.slack.com/archives/C045MDVRB5M/p1666812291441019?thread_ts=1666807588.787899&cid=C045MDVRB5M

Should we just ask in the channel or wait until the milestone? This isn't a blocker so no rush, but if we can get the answer before the meeting we can go ahead and change the meeting and avoid a bit of derailing perhaps

Let's ask in the channel

jbonzo
jbonzo approved these changes Oct 27, 2022
View reviewed changes
Copy link
Collaborator

@jbonzo jbonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my view. Need the codeowner review and let's not merge till we get closer to release side. We don't want this sitting in master and then accidently getting a sem var release before the API is up in prod, that would be confusing to users

@antdjohns antdjohns force-pushed the get-events branch 3 times, most recently from acbeaae to 2d301bb Compare November 14, 2022 14:53
@antdjohns
Added get_events and tests to client library.
1dd23f8 
RV1:
- added get_events function to client library
- added tests.

RV2:
- changed get_ticker_events to get_ticker_history

RV3:
- changed history to events and modifed classes to meet new requirements.
@danielatpolygonio danielatpolygonio merged commit 9b50d03 into master Nov 28, 2022
@danielatpolygonio danielatpolygonio deleted the get-events branch November 28, 2022 15:52
@danielatpolygonio danielatpolygonio mentioned this pull request Nov 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielatpolygonio danielatpolygonio danielatpolygonio approved these changes

@jbonzo jbonzo jbonzo approved these changes

@mmoghaddam385 mmoghaddam385 mmoghaddam385 approved these changes

@clickingbuttons clickingbuttons Awaiting requested review from clickingbuttons

@Darcy-Linde Darcy-Linde Awaiting requested review from Darcy-Linde

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@antdjohns @jbonzo @danielatpolygonio @mmoghaddam385