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

Add quote streaming #108

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

bguban
Copy link
Collaborator

@bguban bguban commented Sep 21, 2021

Still WIP but would be nice if you can take a general look

@bguban bguban requested a review from dblock September 21, 2021 20:48
@bguban bguban changed the title [WIP] Added base functionality [WIP] Add quote streaming Sep 21, 2021
Copy link
Owner

@dblock dblock left a 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/cloud/request.rb Show resolved Hide resolved
@@ -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'
Copy link
Owner

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.

Copy link
Collaborator Author

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.

Copy link
Owner

@dblock dblock Sep 22, 2021

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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?

Copy link
Owner

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(...)

Copy link
Collaborator Author

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:

  1. it will require us to make a major release because we change the namespace and every developer must update their code
  2. 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.

Copy link
Owner

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.

@bguban bguban changed the title [WIP] Add quote streaming Add quote streaming Sep 22, 2021
Copy link
Owner

@dblock dblock left a 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 = ''
Copy link
Owner

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.'),
Copy link
Owner

Choose a reason for hiding this comment

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

remove this

lib/iex/cloud/request.rb Show resolved Hide resolved
lib/iex/cloud/request.rb Show resolved Hide resolved
Copy link
Collaborator Author

@bguban bguban left a 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?

lib/iex/cloud/request.rb Show resolved Hide resolved

Streams quotes in real-time.

`interval` option lets you limit amount of updates. Possible values: `1Second 5Second 1Minute`
Copy link
Owner

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.

@dblock
Copy link
Owner

dblock commented Sep 28, 2021

See my comment. I think we're trying to merge two separate APIs into one and shouldn't.

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

Successfully merging this pull request may close these issues.

None yet

2 participants