Implemented tasklets.synctasklet #58
Conversation
chrisrossi
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
src/google/cloud/ndb/tasklets.py
Outdated
| def synctasklet(*args, **kwargs): | ||
| raise NotImplementedError | ||
| def synctasklet(wrapped): | ||
| """ |
There was a problem hiding this comment.
Although I prefer this style, with the first line of text on its own line, the Google style guide wants for there to be no newline between the triple quote and the first line of text: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html
src/google/cloud/ndb/tasklets.py
Outdated
| raise NotImplementedError | ||
| def synctasklet(wrapped): | ||
| """ | ||
| A decorator to run a function as a tasklet when called. |
There was a problem hiding this comment.
A shortcoming of the original code, in my opinion, is that it's not clear what this actually does from the docstring. I stared at this a while trying to figure out how it was supposed to be used and finally grokked it when I read the narrative documentation.
It seems to me, now, that this docstring is actually inverted from the truth: It's a decorator to run a tasklet as a function. In other words, the point is to decorate a function written like a tasklet, with yield, so it can be run in a synchronous context. It's a tasklet, but rather than returning a future the way a standard tasklet would, it returns the future's result, effectively converting it to a synchronous call, with any asynchronous calls made internally still being able to potentially run in parallel.
It would be great if the docstring could reflect the reality a little better, so the next developer isn't scratching their head the way I was.
tests/unit/test_tasklets.py
Outdated
| with pytest.raises(NotImplementedError): | ||
| tasklets.synctasklet() | ||
| @tasklets.synctasklet | ||
| def regular_function(value): |
There was a problem hiding this comment.
Given the new understanding above, maybe this should be a generator function with a yield, so we can test the intended use case.
|
Also, it's not clear to me why the Kokoro build is failing, here, but it's failing in the |
|
@michaelawyu Anything we can do to help get this PR to the finish line? We are thankful for the help, and would like to see this getting merged. |
googlebot
added
the
cla: yes
Many apologies for the very late response; I had some health issues recently and didn't get to respond. I have made the requested changes, PTAL. |
No description provided.