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

Add fix for typing #65

Merged
merged 4 commits into from
Aug 12, 2022
Merged

Add fix for typing #65

merged 4 commits into from
Aug 12, 2022

Conversation

gkumbhat
Copy link
Member

@gkumbhat gkumbhat commented Aug 11, 2022

Changes

  • Fixes the issues where an optional dependency class is used for type checking along with other types using a combining type, like Dict or Union.

based on #64

Scenarios

  1. Dict[foo, str]
  2. Union[foo, str]

Typing is another one of the operations that happens at the import time.

In both above scenarios, a hashing is performed under the hood in python that tries to make a unique combination of the presented types. Since we are replacing it with lazy module, without this fix, we are raising error for __hash__ function, and thus the typing at import time, throws error.

@gkumbhat gkumbhat force-pushed the add_fix_for_typing branch 2 times, most recently from 68958df to 3230c1f Compare August 11, 2022 16:08
Copy link
Collaborator

@gabe-l-hart gabe-l-hart left a comment

Choose a reason for hiding this comment

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

This PR has the other changes lumped in, so those should be merged first. I'd also like to see a deterministic __hash__ implementation rather than randint

self._raise()
if not _is_import_time():
self._raise()
return randint(0, 100000000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like using randint here. How about id(self)? Nondeterminism scares me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes. That would be better.. Will change this

@@ -530,3 +544,9 @@ def _get_non_import_modules(cls):
for frame in cls._FrameGenerator()
),
)


def _is_import_time():
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will be caught in the other PR, but this needs a docstring and type hint

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, will add that. This PR would need rebasing after the other one gets merged

@@ -244,7 +242,8 @@ def __delitem__(self, *_, **__):
self._raise()

def __eq__(self, *_, **__):
self._raise()
if not _is_import_time():
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should return a bool if _is_import_time is true

Copy link
Member Author

@gkumbhat gkumbhat Aug 12, 2022

Choose a reason for hiding this comment

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

Ah yes, good point! Like the function __eq__ would need to return bool?

I guess currently it would return None which would be falsey, so may be we should return False if _is_import_time is True?

Copy link
Collaborator

@gabe-l-hart gabe-l-hart left a comment

Choose a reason for hiding this comment

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

Looks good!

@gabe-l-hart gabe-l-hart merged commit 36af643 into IBM:main Aug 12, 2022
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.

2 participants