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

timeout for whole rest operations #14

Merged
merged 8 commits into from
Sep 3, 2015
Merged

Conversation

69736c616d
Copy link
Contributor

here is the simple patch for timeout.

@mrtazz
Copy link
Owner

mrtazz commented Jul 11, 2015

thank you for submitting this pull request and sorry for the wait. Could you remove the DS_Store files and add some unit tests around the functionality so I don't accidentally break it in future commits?

@mrtazz
Copy link
Owner

mrtazz commented Aug 9, 2015

Sorry for the delay here. I just changed the tests for running against httpbin. Would you mind merging master into your pull request and writing a test to run against http://httpbin.org/delay/10 or something with the right timeout settings?

@69736c616d
Copy link
Contributor Author

ok, i will check out that.

Greetings
iyasar

On 10-08-2015 01:03, Daniel Schauenberg wrote:

Sorry for the delay here. I just changed the tests for running against
httpbin. Would you mind merging master into your pull request and
writing a test to run against http://httpbin.org/delay/10 or something
with the right timeout settings?


Reply to this email directly or view it on GitHub
#14 (comment).

@69736c616d
Copy link
Contributor Author

corrected code where test cases fail.

@69736c616d
Copy link
Contributor Author

checking other issues,

@69736c616d
Copy link
Contributor Author

i think, it is done

@mrtazz
Copy link
Owner

mrtazz commented Aug 10, 2015

@69736c616d thanks for the quick update! I would still love to have some tests for the new functionality against the httpbin endpoint I mentioned. This is important for two reasons:

  1. I don't want to accidentally break functionality in the future
  2. There should be a documented way of how to use it

The tests provide both of those things. For extra bonus points, some documentation in the README would be great.

@69736c616d
Copy link
Contributor Author

@mrtazz http://httpbin.org/delay/10 is not suitable for delete, post and put operations. Therefore i'm getting 405 from server. Can you suggest any other url ?

@mrtazz
Copy link
Owner

mrtazz commented Aug 14, 2015

Ugh I remember there being an open issue to allow the route for all methods. In that case I think it's ok to only have the test for GET for now. And I'll keep an eye on httpbin to see if they implement that.

@mrtazz
Copy link
Owner

mrtazz commented Aug 14, 2015

I just updated the unit test suite in master to also support JSON parsing (#25). So if you merge from master you should also be able to parse the JSON output you get back from httpbin

@mrtazz
Copy link
Owner

mrtazz commented Sep 3, 2015

Thanks for taking the time to fixing this up! Looking good now!

mrtazz added a commit that referenced this pull request Sep 3, 2015
timeout for whole rest operations
@mrtazz mrtazz merged commit 34cc7ef into mrtazz:master Sep 3, 2015
@mrtazz mrtazz added this to the v0.3.0 milestone Sep 3, 2015
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