Make gevent and gevent_uswgi async drivers compatible with threading.… - #245
Make gevent and gevent_uswgi async drivers compatible with threading.…#245dedoussis wants to merge 2 commits into
Conversation
| def __init__(self, target, args=[], kwargs={}): | ||
| super(Thread, self).__init__(target, *args, **kwargs) | ||
|
|
||
| def _run(self): |
There was a problem hiding this comment.
This is not part of the threading.Thread interface.
There was a problem hiding this comment.
And why is this a problem? The Thread class in this driver is used internally.
There was a problem hiding this comment.
I see. I couldn't find any occurrences of this method being used anywhere in the codebase and since this is a private method, I decided to not include it anymore. If that's not the case, I'm more than happy to put it back 👍
There was a problem hiding this comment.
Sorry, my question wasn't directly related to the _run() method, I was wondering about the whole change. I don't see any benefit in switching to the Thread class instead of using the Greenlet class as I'm doing here. There's nothing wrong with with the Thread class as far as I know, but my solution based on Greenlet has years of testing, so I'm not sure it makes sense to switch to a different implementation without a good reason.
But in any case, you asked about the _run() method. This is where you put the code that runs in the greenlet. You can see how this works in the documentation.
There was a problem hiding this comment.
Thanks for the _run() clarification, that makes complete sense then. I will include it in the class, if we decide to go forward with this PR.
Codecov Report
@@ Coverage Diff @@
## main #245 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 1894 1896 +2
Branches 343 343
=========================================
+ Hits 1894 1896 +2
Continue to review full report at Codecov.
|
|
As stated in the issue, I don't really see what is the improvement this change brings to the project. Did you have any problems with the current implementation? |
|
I will not be merging this change, as discussed in #244. |
…Thread interface
Fixes: #244