-
Notifications
You must be signed in to change notification settings - Fork 27
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
Type hinting overhaul. #219
Conversation
5b1f7c1
to
5a3bf7b
Compare
70336ac
to
b0dcde9
Compare
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.
Nice work!
@@ -39,7 +44,7 @@ class ObjectStoreException(Exception): | |||
class ObjectAlreadyExists(ObjectStoreException): | |||
"""Exception raised when trying to store a DiffSyncModel or DiffElement that is already being stored.""" | |||
|
|||
def __init__(self, message, existing_object, *args, **kwargs): | |||
def __init__(self, message: str, existing_object: Union["DiffSyncModel", "DiffElement"], *args: Any, **kwargs: Any): |
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.
Should existing_object
just be an Any
?
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 other than DiffSyncModel
or DiffElement
would we store in a store?
b0dcde9
to
862fea9
Compare
862fea9
to
75b6b19
Compare
Addressed all outstanding comments, rebased against develop and put it into a backwards-compatible state. Should be an easy-ish merge at this point I think? |
75b6b19
to
acea842
Compare
acea842
to
4fa39b5
Compare
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.
LGTM
Closes #175. Things that happened here:
disallow_untyped_defs
in mypy config inpyproject.toml
Mapping
withDict
,Collection
withList
andText
withstr
, because all our mappings are dicts anyway, collections are lists (3.9 could usedict
/list
, maybe we can drop 3.8 support with 2.0?) andText
is only needed for backwards compatibility with Python 2.x which we aren't offering