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

Updated Stream Instructions for sip parameter in README #615

Merged

Conversation

hinchcliffz
Copy link
Contributor

@hinchcliffz hinchcliffz commented May 3, 2022

I updated the Stream instructions to be more clear.

  • They had an unneeded URL in the README.md updated it to reflect the proper way to call it.
  • They had SIP parameter in all capitals in the instructions as well. Fixed this to reflect the proper way to call it.

Copy link
Contributor

@drew887 drew887 left a comment

Choose a reason for hiding this comment

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

Hey @hinchcliffz thank you for taking the time to open a PR!

They had an unneeded URL in the README.md updated it to reflect the proper way to call it.

I wouldn't say that you changed it to the "proper" way to call it. We do annotate the base_url parameter as a URL type so having it be passed as the URL like it already was would technically be the "proper" way to call it; and it also ensures that what you're passing in is a valid url.

I think I'd rather we keep these examples to have the URL parts there just to help ease any issues that might come up with users accidentally making typos or the like.

If you'd like us to be explicit about the fact that technically you could pass in a raw str value here, then we should update the type annotations on the Stream and TradingStream classes to show it can be a Union[URL, str] in this PR 🤔

README.md Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented May 3, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@hinchcliffz
Copy link
Contributor Author

That makes a lot more sense @drew887. I added the URL back in and added the additional comment in the README.md.

@hinchcliffz hinchcliffz requested a review from drew887 May 3, 2022 19:52
@drew887 drew887 changed the title Updated Stream Instructions to not use unneeded URL Updated Stream Instructions for sip parameter in README May 3, 2022
@drew887 drew887 merged commit f2c0462 into alpacahq:master May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants