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

Allow adjusting hackney options #203

Merged
merged 1 commit into from Apr 26, 2017

Conversation

aaronrenner
Copy link
Member

I was having an issue where BrowserStack wasn't sending me a response on some of my commands, causing my script to just hang. This change will allow me to override the hackney options and set custom timeouts.

I was having an issue where BrowserStack wasn't sending me a response on
some of my commands, causing my script to just hang. This change will
allow me to override the hackney options and set custom timeouts.
@aaronrenner
Copy link
Member Author

Actually, now that I think about it, maybe this setting should be on the driver level.

config :wallaby, Wallaby.Phantom, 
  hackney_options: [timeout: :infinity, recv_timeout: :infinity]

What do you think?

@keathley
Copy link
Member

Setting it per driver seems like a good call to me.

Copy link
Collaborator

@PragTob PragTob left a comment

Choose a reason for hiding this comment

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

Some small formulation questions.

Otherwise it looks fine to me. What would be to use case for per driver configuration? For me this looks fine and I'd just go to the next complexity level if we really needed that extra layer.


### Adjusting timeouts

Wallaby uses [hackney](https://github.com/benoitc/hackney) under the hood, so we
Copy link
Collaborator

Choose a reason for hiding this comment

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

technically it uses httpoison which uses hackney, should be fine though - was just a bit confusing to me.


# Overriding a value
config :wallaby,
hackney_options: [timeout: 5_000]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this overrides the complete options (not just a value)? Or does config do a merge of some kind on the values? I think it doesn't do merging...

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried out this behavior locally using IO.pry/1 and confirmed my project config setting was merged in with the defaults provided in Wallaby's config.exs.

@PragTob
Copy link
Collaborator

PragTob commented Apr 26, 2017

Haha and there we disagree :D I'd still like to get the reasoning, but am fine with it as you two seem to be in agreement :)

@keathley
Copy link
Member

@PragTob I could go either way on it. The only reason I thought breaking it out per driver might be useful is so each driver could specify their own timeouts. I'm not sure that makes sense or not at this point. I'm totally fine with a single config for now.

@aaronrenner
Copy link
Member Author

@PragTob I think you're right that we don't need per-driver configuration. Unless you'd like any other changes, this should be good to merge, as-is.

@PragTob
Copy link
Collaborator

PragTob commented Apr 26, 2017

:shipit:

@PragTob PragTob merged commit ebbbde8 into elixir-wallaby:master Apr 26, 2017
@aaronrenner
Copy link
Member Author

Thanks @PragTob. 👍

@aaronrenner aaronrenner deleted the ar-hackney-options branch April 27, 2017 22:23
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

3 participants