-
Notifications
You must be signed in to change notification settings - Fork 12
Entity hub: Add type hints #270
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
Conversation
antirotor
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.
It is massive and Github is veeery slow with it - some suggestions:
When there are methods like get_folder_by_id() that should return Optional[FolderEntity] but are returning the result of generic get_or_fetch_entity_by_id() that returns BaseEntity - I suggest using cast():
from typing import cast
class EntityHub:
...
def get_folder_by_id(
self,
entity_id: str,
allow_fetch: bool = True,
) -> Optional[FolderEntity]:
"""Get folder entity by id.
Args:
entity_id (str): Folder entity id.
allow_fetch (bool): Try to fetch entity from server if is not
available in cache.
Returns:
Optional[FolderEntity]: Folder entity object.
"""
if allow_fetch:
entity = self.get_or_fetch_entity_by_id(entity_id, ["folder"])
return cast(Optional[FolderEntity], entity)
return self._entities_by_id.get(entity_id)This is on few places. The rest of it is this kind of problem we've discussed before:
status: Optional[str] = UNKNOWN_VALUEwhere correct way would be:
status: Union[Optional[str], _CustomNone]] = UNKNOWN_VALUEI still believe in proper annotation even of sentinel types, but please ignore if you don't agree.
There is a number of MyPy errors - mostly about things mentioned above and some in the realm of variable initialized as None but later on used as different type.
I suggest running MyPy and correct/ignore what it produce.
Changelog Description
Add type hints to entity hub and use f-string formatting.
Additional review information
I've tried as much as I could. Doing the type hints here was the most difficult. Please, unless there is something "wrong" don't suggest more enhancemenets.
Testing notes: