-
Notifications
You must be signed in to change notification settings - Fork 63
chore(types): Add type hints and hierarchy to Structure types #1036
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: main
Are you sure you want to change the base?
Conversation
danielballan
left a comment
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.
Great!
I left a suggestion, a question, and an opinion for you to consider by freely pass over as you will. :-)
| class Structure(ABC): | ||
| @classmethod | ||
| # TODO: When dropping support for Python 3.10 replace with -> Self | ||
| def from_json(cls, structure: Mapping[str, Any]) -> "Structure": |
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'm OK with this, but I would prefer not to.
When I see:
class COOStructure:
...I know that everything I need to read is in front of me. When I see:
class COOStructure(Structure):
...I have to cross-reference. The cross-referencing is worthwhile if we can avoid significant repetition. But given that the among of DRY-ness that this achieves is quite small, I would be happier repeating this out in the specific classes.
However, YMMV, and if this seems much better to Team DLS, I do not object.
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.
After #1036 I want to change e.g.
STRUCTURE_TYPES = OneShotCachedMap(...)
# to
STRUCTURE_TYPES = OneShotCachedMap[StructureFamily, type[Structure]](...)When we then access the types inside, I want typing and my IDE to help me out with
structure_type = STRUCTURE_TYPES[attributes["structure_family"]]
self._structure = structure_type.from_json(attributes["structure"])Structure could become a Protocol here, since it's just a contract that the Map will only contain types that can be deserialised.
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.
From trying this out, it seems Pydantic is not happy if you try and use protocols with a generic BaseModel with bounds, so I've had to revert to an ABC.
class Proto(Protocol):
...
P = TypeVar("P", bound=Proto)
class DataSource(BaseModel, Generic[P]):
t: Optional[P]
tiled/structures/awkward.py
Outdated
|
|
||
| def project_form(form, form_keys_touched): | ||
| def project_form( | ||
| form: awkward.forms.Form, form_keys_touched |
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.
| form: awkward.forms.Form, form_keys_touched | |
| form: awkward.forms.Form, form_keys_touched: List[str] |
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.
form_keys_touched = set(report.data_touched)Looks like it's a set in the only instance it's used. I'll make it an Iterable to prevent issues.
f22208e to
93d48c8
Compare
30d6f59 to
296fefe
Compare
296fefe to
a10ff04
Compare
Prep work for #689
Checklist