-
-
Notifications
You must be signed in to change notification settings - Fork 60
Add quote streaming #108
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
base: master
Are you sure you want to change the base?
Add quote streaming #108
Conversation
8765a87
to
17c2b0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it’s pretty close.
lib/iex/api/config/client.rb
Outdated
@@ -24,6 +25,7 @@ def reset! | |||
self.ca_file = defined?(OpenSSL) ? OpenSSL::X509::DEFAULT_CERT_FILE : nil | |||
self.ca_path = defined?(OpenSSL) ? OpenSSL::X509::DEFAULT_CERT_DIR : nil | |||
self.endpoint = 'https://cloud.iexapis.com/v1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t love that we’re adding another parameter here. We’re modifying the endpoint, not adding another one. Maybe because there’s just one API that goes to a different endpoint, hardcore that in the request and allow me to overwrite it if I want to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to use endpoint.gsub('://cloud.', '://cloud-sse.')
to generate the new endpoint. It allows users to specify the API version in the endpoint
config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like the least future proof option of all :)
I was thinking that stream_quote
would do this:
def stream_quote(symbols, options = {})
options = { token: secret_token, endpoint: '...cloud-sse...' }.merge(options)
options[:symbols] = Array(symbols).join(',')
interval = options.delete(:interval)
get_stream("stocksUS#{interval}", options) do |payload|
payload.each do |quote|
yield IEX::Resources::Quote.new(quote)
end
end
This way if that endpoint does change, one can override it in the request without releasing a new version of the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed your comment. IEX supports a lot of other streaming endpoints. For example, https://iexcloud.io/docs/api/#cryptocurrency is for streaming crypto. I don't think that it's a good idea to hard code it in every method that will use streaming.
Again, as I understand we have this endpoint
configuration to give the ability to use a sandbox or specify a specific API version, but if we hard code that then it will require people to set sandbox API in the code instead of the configuration.
I checked the sandbox documentation https://iexcloud.io/docs/api/#testing-sandbox and it looks like my current solution will not work with the sandbox env at all. So I think that the first implementation (where we had an extra config option) is the best. It allows us to specify different URLs for different environments, it's straight and simple. And if IEX decides to change the URL (which I doubt very much) then developers will be required just to update their config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dblock so do you want me to hard code the URL or is it still an open discussion? if we are going to hard code then what do you suggest to do with a sandbox URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think you're right. SSE is a thing on its own.
I think we should build SSE support next to the current endpoint implementation rather than attempting to bolt it on top. This means namespacing SSE support under IEX::Api::Streaming
or IEX::Streaming::API
or IEX:SSE::API
- no strong preference on which one.
SSE would have its own implementation, its own connection/request/etc. Much of it can be shared from the existing implementation, meaning you'll want to extract some base classes and potentially move IEX::Api into IEX::Web::Api
or IEX::Rest::Api
.
This is what I think we want to write:
IEX::Streaming::Api.configure do |config|
config.endpoint = 'https://.... sse
end
IEX::Streaming::Client.new(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that it's worth the effort:
- it will require us to make a major release because we change the namespace and every developer must update their code
- we will decrease readability when trying to share the functionality. there will be a lot of
super
calls with small updates.
And honestly say I don't see any benefits from separating the functionality to different namespaces. The only difference in the code is request.rb
file that requires this event parser
. So maybe it makes sense to create ansse_request.rb
file that will be responsible for streaming (or just continue using the request.rb
). Or we can try to implement a Faraday middleware that will take event parser
responsibility.
Probably it makes sense to ask other contributors their opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just think it would make things a lot cleaner and I expect IEX to keep adding endpoints. If you don't feel like doing it, I might pick it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- needs README
def get(path, options = {}) | ||
request(:get, path, options) | ||
end | ||
|
||
def get_stream(path, options = {}) | ||
buffer = '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use StringIO
end | ||
|
||
options = { | ||
endpoint: endpoint.gsub('://cloud.', '://cloud-sse.'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs README
I added a description for the api. Do you want to add any thing extra?
|
||
Streams quotes in real-time. | ||
|
||
`interval` option lets you limit amount of updates. Possible values: `1Second 5Second 1Minute` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interval
... so it's a complete sentence.
Possible values are ... and quote each value. End with a period.
See my comment. I think we're trying to merge two separate APIs into one and shouldn't. |
Still WIP but would be nice if you can take a general look