Skip to content
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

Merged
merged 1 commit into from
Aug 18, 2023
Merged

Type hinting overhaul. #219

merged 1 commit into from
Aug 18, 2023

Conversation

Kircheneer
Copy link
Collaborator

@Kircheneer Kircheneer commented Mar 17, 2023

Closes #175. Things that happened here:

  • Fully type hint all function/method definitions
  • Active disallow_untyped_defs in mypy config in pyproject.toml
  • Replacing Mapping with Dict, Collection with List and Text with str, because all our mappings are dicts anyway, collections are lists (3.9 could use dict/list, maybe we can drop 3.8 support with 2.0?) and Text is only needed for backwards compatibility with Python 2.x which we aren't offering

@Kircheneer Kircheneer added this to the 2.0.0 milestone Mar 17, 2023
@Kircheneer Kircheneer force-pushed the typing-overhaul branch 2 times, most recently from 70336ac to b0dcde9 Compare March 17, 2023 14:53
Copy link
Collaborator

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

diffsync/__init__.py Outdated Show resolved Hide resolved
diffsync/__init__.py Show resolved Hide resolved
diffsync/__init__.py Show resolved Hide resolved
@@ -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):
Copy link
Collaborator

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?

Copy link
Collaborator Author

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?

pyproject.toml Outdated Show resolved Hide resolved
@Kircheneer
Copy link
Collaborator Author

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?

@Kircheneer Kircheneer modified the milestones: 2.0.0, 1.8.0 Aug 15, 2023
@Kircheneer Kircheneer marked this pull request as ready for review August 17, 2023 15:12
@Kircheneer Kircheneer requested a review from chadell as a code owner August 17, 2023 15:12
Copy link
Collaborator

@chadell chadell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Kircheneer Kircheneer merged commit 2c1577f into develop Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type hinting setup overhaul
3 participants