Initial example of Python TypeSystem#58
Conversation
Codecov Report
@@ Coverage Diff @@
## golden #58 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 17 +1
Lines 583 619 +36
Branches 72 74 +2
=========================================
+ Hits 583 619 +36
Continue to review full report at Codecov.
|
JacobHayes
left a comment
JacobHayes
left a comment
There was a problem hiding this comment.
Thanks for putting this up so quick!
Do you mind adding src/arti/types/python.py, think that was missed. 😅
I don't feel so bad about checking types/instances specifically (the purpose of this code is type checking 😁), but I agree w/ the unwieldiness matching to the Adapter's external type (and that interface overall). Still processing a few more thoughts/paths, but might wait for python.py
tests/arti/types/test_types.py
Outdated
| assert type(python.to_core(int())) == Int64 | ||
| assert python.from_core(Int64()) == int |
There was a problem hiding this comment.
Hmm, int() is 0 so shouldn't int() be int in the first line (akin to what from_core returns)?
Also, think we can tidy these a little bit (but still fundamentally type checking):
| assert type(python.to_core(int())) == Int64 | |
| assert python.from_core(Int64()) == int | |
| assert isinstance(python.to_core(int), Int64) | |
| assert python.from_core(Int64()) is int |
(I think the is may be slightly better than == since it's a singleton, but 🤷)
--
I think there's still a fundamental design oddity here as you note.
There was a problem hiding this comment.
I think the tidying is fine, but we have in https://github.com/replicahq/artigraph/blob/cb0283674a148a3b9315ae7bc5df071898449fef/src/arti/types/core.py#L131
self.adapter_by_external_priority[type(type_)].to_internal(type_)
i.e., checking the type of type_, because that's how other (non Python) type systems work (e.g., pyarrow), so you want to pass int() to type(...) not int.
mikss
changed the title
|
@JacobHayes I think this is ready now; after thinking over some of our proposed solutions, I think just allowing a Python-weirdness-specific release valve in the form of "check if metaclass" (see this commit b1f6ea3) is lightest weight. |
JacobHayes
left a comment
JacobHayes
left a comment
There was a problem hiding this comment.
Looking much better overall, but I don't think the strictly type-based matching (even with system_metaclass) will work for everything: BigQuery's SchemaField is a single type with an instance level field_type string attribute. Should we call this good for now or try to iron out?
JacobHayes
left a comment
JacobHayes
left a comment
There was a problem hiding this comment.
Thanks for iterating though this, it's in a much better state now! Gonna tidy commits up a bit and merge. 🚀
JacobHayes
force-pushed
the
trivial_artifact
branch
from
43d6199 to
809878b
Compare
JacobHayes
changed the title
2 participants
This is a first attempt at what we talked about yesterday w.r.t. types. I do think there needs to be some additional thought here, since I don't like the fact that we need to frequently compare type classes and instances. E.g.,