Allow bypassing futuroscope entirely #109
Conversation
| @@ -1,8 +1,9 @@ | |||
| ### 0.8.2 (Next) | |||
|
|
|||
| This version is no longer tested with Ruby < 2.2. | |||
| # This version is no longer tested with Ruby < 2.2. | |||
There was a problem hiding this comment.
I know this is just for Danger, but maybe make it 4 ####.
|
|
||
| ```ruby | ||
| api = Hyperclient.new('https://grape-with-roar.herokuapp.com/api') do |client| | ||
| client.async_options = {enabled: false} |
There was a problem hiding this comment.
Minor: make it Ruby pretty, add a space after { and before }.
| @@ -39,6 +39,7 @@ class EntryPoint < Link | |||
| def initialize(url, &_block) | |||
| @link = { 'href' => url } | |||
| @entry_point = self | |||
| @async_options = {enabled: true} | |||
There was a problem hiding this comment.
rubocop -a will fix this too
| response = Futuroscope::Future.new do | ||
| @entry_point.connection.run_request(method, _url, body, nil) | ||
| end | ||
| do_async = @entry_point.async_options[:enabled] |
There was a problem hiding this comment.
Minor, but this isn't used in more than one place, so I would just do if @entry_point.async ....
There was a problem hiding this comment.
I've updated this to do if @entry_point.options[:async] directly. Would you prefer I add an explicit async getter allowing this code to do if @entry_point.async instead?
|
First, thank you, this is great. I feel like this should just be |
|
Thanks for the feedback! I'll make the changes you suggested asap :) |
This addition will be used in the Link to control if requests are done using futuroscope in a background thread or not.
This allows requests to bypass futuroscope entirely, rather than needing to rely on setting the global pool to NoPool.
Tweak changelog formatting to avoid warnings.
|
@dblock care to re-review? 😄 |
|
Merging this, thanks! Made minor comments if you want to address them in a future PR feel free. |
|
|
||
| ```ruby | ||
| api = Hyperclient.new('https://grape-with-roar.herokuapp.com/api') do |client| | ||
| client.options.merge!(async: false) |
There was a problem hiding this comment.
No need to change this, but in Ruby this is an antipattern. All you're doing is setting async, so client.options[:async] = false is a much clearer operation than merge.
| # Public: Read options. | ||
| # | ||
| # Returns a Hash. | ||
| attr_reader :options |
There was a problem hiding this comment.
These should be combined as attr_accessor, no?
There was a problem hiding this comment.
I separated them to have documentation on both, but it may not be useful enough to warrant that distinction.
As discussed in #102 there are cases where we don't really want the async behavior provided by futuroscope.
In @Talkdesk's case, we're using hyperclient with the manticore gem that already has its own worker thread pool.
While the proposed
NoPoolapproach kind-of works, that approach is global. This means that you chose once for your entire system whether you have a pool or not, and that choice impacts all hyperclient instances (even communicating with different systems) and futuroscope users (unless they allow you to override the pool, which for instance hyperclient doesn't).Since that is rather heavy-handed, instead this PR adds an
async_optionsto theEntryPoint, which allows hyperclient uses to per-instance specify if they want requests to be asynchronous or not. By default, the previous async behavior is followed, but this can be changed easily.In the future I hope to also expand the
async_optionsso you can specify a specific pool, so that you can have different pools for different hyperclient instances.