First draft to show how to make syntactically nicer with type annotations #696
Conversation
|
Can one of the admins verify this patch? |
|
Hey, this looks neat, i'd definitely merge it if it matures. Thanks for the effort! Two points:
What do you think? |
|
So potentially just the first example but blended into the rpc decorator? Funnily enough, I did that first, but then realized there were many other kwargs I hadn't seen before hence allowing it to have optional pass through kwargs I think I understand what you mean, but if you could add some dummy examples it would help :) |
|
The following is OK: class HelloWorldService(ServiceBase):
@rpc(_is_async=True)
def say_hello(
ctx,
name: Unicode,
times: UnsignedInteger32,
a: Iterable(Decimal),
b: Iterable(Iterable(Decimal)),
) -> Iterable(Unicode):
for _ in range(times):
yield f"Hello, {name}"The following should fail with: "*params must be empty when type annotations are used" class HelloWorldService(ServiceBase):
@rpc(Unicode, _is_async=True)
def say_hello(
ctx,
name: Unicode,
times: UnsignedInteger32,
a: Iterable(Decimal),
b: Iterable(Iterable(Decimal)),
) -> Iterable(Unicode):
for _ in range(times):
yield f"Hello, {name}"The following usage of class HelloWorldService(ServiceBase):
@rpc
def say_hello(
ctx,
name: Unicode,
times: UnsignedInteger32,
a: Iterable(Decimal),
b: Iterable(Iterable(Decimal)),
) -> Iterable(Unicode):
for _ in range(times):
yield f"Hello, {name}"The following (with parens) should work same as above: class HelloWorldService(ServiceBase):
@rpc()
def say_hello(
ctx,
name: Unicode,
times: UnsignedInteger32,
a: Iterable(Decimal),
b: Iterable(Iterable(Decimal)),
) -> Iterable(Unicode):
for _ in range(times):
yield f"Hello, {name}"How we handle missing data:
The following falls under the first point above. class HelloWorldService(ServiceBase):
@rpc()
def say_hello(
ctx,
name,
times: UnsignedInteger32,
a: Iterable(Decimal),
b: Iterable(Iterable(Decimal)),
) -> Iterable(Unicode):
for _ in range(times):
yield f"Hello, {name}"You needn't worry about |
|
The following should fail with: "_returns must be omitted when type annotations are used. Please annotate the return type" class HelloWorldService(ServiceBase):
@rpc(_returns=Iterable(Unicode), _is_async=True)
def say_hello(
ctx,
name: Unicode,
times: UnsignedInteger32,
a: Iterable(Decimal),
b: Iterable(Iterable(Decimal)),
):
for _ in range(times):
yield f"Hello, {name}" |
|
I've added this exception handling, will try to add some tests |
|
I've blindly added some tests :D couldnt seem to get the |
|
run_tests.sh is quite involved -- it starts by downloading and compiling the desired cpython version, so requires all C tooling to be ready to go. The good news is you don't need it, just switch to your local virtualenv run the tests directly. There is a readme for tests under spyne/test. It was embarrassingly out of date but I just updated it. Let me know if you still have questions.
Spyne project predates those niceties by at least a decade :) |
|
Hmmm |
|
Jenkins seems to fail to get the repo
|
|
don't worry about these, just use pytest to run your specific tests |
|
Done ✅ - I needed to delete |
Looks good so far. Next, you need to fuse the functionality of
As said in spyne/test/README.md, You can avoid all that by calling |
|
So... This following use the existing rpc functionality and should all be backward compatible # All existing with no type annos
class HelloWorldService(ServiceBase):
@rpc(Unicode,UnsignedInteger32, Iterable(Decimal), Iterable(Iterable(Decimal)), _returns=Iterable(Unicode), _is_async=True)
def say_hello(ctx, name, times, a, b):
for _ in range(times):
yield f"Hello, {name}"# Support existing peoples code where they have full type annotations
class HelloWorldService(ServiceBase):
@rpc(Unicode, UnsignedInteger32, Iterable(Decimal), Iterable(Iterable(Decimal)), _returns=Iterable(Unicode), _is_async=True)
def say_hello(
ctx,
name: Unicode,
times: UnsignedInteger32,
a: Iterable(Decimal),
b: Iterable(Iterable(Decimal)),
): # Note - no return defined
for _ in range(times):
yield f"Hello, {name}"# Support existing peoples code where they have partial type annotations
class HelloWorldService(ServiceBase):
@rpc(Unicode, UnsignedInteger32, Iterable(Decimal), Iterable(Iterable(Decimal)), _returns=Iterable(Unicode), _is_async=True)
def say_hello(
ctx,
name: Unicode,
times: UnsignedInteger32,
a,
b: Any,
): # Note - no return defined
for _ in range(times):
yield f"Hello, {name}"BUT, if they dont supply any args to |
plq
force-pushed
the
feature/typed-rpc
branch
2 times, most recently
from
3840c4e to
14c7e04
Compare
plq
force-pushed
the
feature/typed-rpc
branch
from
14c7e04 to
1b3ff88
Compare
|
OK I seem to have brought back all functionality lost to bitrot. |
This should work as before
The logic to support this will be too complicated, so no need to bother. Users need to choose between having type annotations in function definition or in the
Same as above, this isn't supported. |
|
Ok, this next release won't be backward compatible for anyone who had type annotations in their functions? |
Is this a thing? People have type annotations in their functions decorated by |
|
I'm going to say there's a chance people have done - allows for IDE autocomplete and able to be type checked using mypy Perhaps some kind of depreciation notice, either way we will probably need to tackle the slightly more complex scenarios above |
|
OK, this is a good point. hmmm. First, mypy's type annotators are not compatible with spyne's type annotators. So users will have to choose one or the other. Maybe instead of
we should implement
Or, we could make sure Spyne's annotators are compatible with mypy's, though I imagine this would open a huge can of worms so I'm not entirely willing to go down that path. A third option would be to merge this patch as-is (we'd have to rename |
Converting mypy types to spyne types while reading annotations inside |
|
Yes this was the next step after supporting type annotations inline - I played around with this mapping but having read your docs there are suggestions on things like float/double/decimal and I feel like most people will just say float without reading the docs where you suggest using decimal. I think it might be best going forward to make the breaking change to be honest. So long as it's clear in the versioning and docs. It's an easy change for the user to remove redundant code (if they were type hinting in multiple locations) - they'll be happy for it. |
|
Any further thoughts? |
|
Sorry, yes, I think we should parse annotations like we originally wanted. If backwards compatibility becomes a problem, we can turn off reading annotations with eg by a new |
|
I've given it a go
import spyne.const
spyne.const.READ_ANNOTATIONS = False
# Now import rest of spyne functionality
from spyne import (
Decimal,
Iterable,
ServiceBase,
UnsignedInteger32,
)
from spyne import rpc
class HelloWorldService(ServiceBase):
@rpc
def say_hello(
ctx,
name: UnsignedInteger32,
times: UnsignedInteger32,
a: Iterable(Decimal),
b: Iterable(Iterable(Decimal)),
) -> None:
for _ in range(times):
yield f"Hello, {name}" |
|
Besides the PEP8 violations, this looks good to me. If you could make sure lines don't exceed 80 characters, I can merge this |
|
Ran through black, but looks like it changed quite a bit to align with PEP8 |
|
Might be good to add some doccos too, not sure where the best place to add them is, or what your plan is going forward with doccos - latest doccos still say they're WIP? |
Huh, this is a mess. Can't you simply break lines manually? |
The apidocs are all we have. You are free to add additional material under the docs section though. |
I can, though I'd reccomend using something like black over your whole repo to align and enforce a standard. Maybe a seperate PR ? |
ghandic
force-pushed
the
feature/typed-rpc
branch
from
fee03db to
0165753
Compare
|
done ✅ |
|
Anything remaining for this? |
|
Sorry, release crunch at $DAYJOB, going to look at this first thing I find some time |
Just the initial example - dont want to go too far down the rabbit hole if there's no appetite. Open to thoughts
Example
Example 2