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

Update README timeout #3115

Merged
merged 1 commit into from Feb 26, 2019
Merged

Conversation

jaredsuttles
Copy link
Contributor

PR Checklist:

  • I have run npm test locally and all tests are passing.
  • I have added/updated tests for any new behavior.
  • If this is a significant change, an issue has already been created where the problem / solution was discussed: [N/A]

PR Description

Timeout argument description is currently very misleading. It was split into two bullets here but the note regarding OS-wide timeout does not apply to the first bullet, but rather the second. In addition, this change made the wording more confusing, in my opinion. I reverted it back to what it was originally.

@reconbot
Copy link
Contributor

Reads pretty good

@reconbot reconbot merged commit 7195b50 into request:master Feb 26, 2019
@lalau
Copy link

lalau commented Mar 6, 2019

I am still confused with the timeout doc. Does the value apply to both bullet points (read timeout and connection timeout)? Or can it set different value to them?

@jaredsuttles
Copy link
Contributor Author

@lalau "integer containing number of milliseconds, controls two timeouts." then those two timeouts are listed below. So yes, it applies to both. I think it makes sense now but maybe you have a suggestion for further clarity?

sthnaqvi added a commit to sthnaqvi/request that referenced this pull request Mar 31, 2019
docs: adjust readme timeout argument description to be clearer (request#3115)
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