search

NebulousLabs / python-skynet

Library for integrating Skynet with Python applications
MIT License
19 stars 11 forks source link

Easy-to-review timeout support #25

Closed xloem closed 4 years ago

xloem commented 4 years ago

These are my local changes to support timeout.

20 adds more changes on top of this and would need a trivial rebase if this is merged.

mrcnski commented 4 years ago

Hey @xloem, thanks for the contribution! Looks good, just a few things:

  1. I just merged a change fixing minor errors, so you'll have to resolve conflicts with master.
  2. I'm not sure I like having timeout be another optional argument. It makes it impossible to pass in a timeout without also passing in opts. Have you considered just making the timeout a field in opts?
  3. If you're able to submit some kind of evidence that this works (e.g. asciinema recording) it would help the reviewers a lot, as we don't have any kind of automated testing yet.

Thanks!

xloem commented 4 years ago

Thanks so much for the speedy response. I'll try to keep the things you mention in mind but won't be updating the PR for now due to other things going on. I'll make a new one or reopen this if I get back to this.

Do you have time to review #20 which also mentions timeout?

atm python-requests can hang indefinitely sometimes, without a timeout. The parameter should be passed internally even if there is no interface to change it yet.