-
Notifications
You must be signed in to change notification settings - Fork 49
Add cast function. #944
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
base: master
Are you sure you want to change the base?
Add cast function. #944
Conversation
d509ac2
to
0327ede
Compare
4aff5a3
to
01836fa
Compare
2b68a56
to
e383581
Compare
Is the idea here that we support generating casts between any two types, and then we let the server deal with figuring out which are real? That's probably a reasonable first take. |
f473ef1
to
67d8d2f
Compare
if gt in COLLECTION_TYPES: | ||
# Import collection classes directly so mypy sees them as | ||
# generic classes, not type aliases | ||
self.write( | ||
f"from {rel_import.module} " | ||
f"import {type_ident} as {type_ident}" | ||
) | ||
else: | ||
self.write(f"{type_ident} = {imported_name}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if you don't do this?
I think it shouldn't matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mypy complains about applying the type arguments to std.tuple
without this:
tests/test_qb_cast_tuple_01.py:20: error: Bad number of arguments for type alias, expected 0, given 4 [type-arg]
It's ok with std.array
though, for some reason...
self.write() | ||
self.write_type_reflection(stype) | ||
|
||
# cast method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe generate these in a separate mtehod?
tests/test_qb.py
Outdated
# scalar to enum | ||
from models.orm import default, std | ||
|
||
result = self.client.get(default.Color.cast(std.str("Red"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example of where I feel the explicit cast
is inconsistent. We are casting a Python literal string "Red"
to std.str
via a (Pythonic) call to a constructor, whereas the second cast is via a classmethod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this need a bit more work. Ideally we should expose all explicit casts with support for base Python types, e.g the Enum casts should accept Python strings directly without an extra std.str
cast.
Allows users to cast between types:
Supports scalars, enums, array, tuple, range. Multiranges not supported, not sure we can even construct them in the qb right now.