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

Minor type fix #535

Merged
merged 1 commit into from
Aug 13, 2022
Merged

Minor type fix #535

merged 1 commit into from
Aug 13, 2022

Conversation

victorcwai
Copy link
Contributor

Fix type of argument module in load_module in dill/session.py

Fix type of argument `module` in `load_module` in `dill/session.py`
@leogama
Copy link
Contributor

leogama commented Aug 6, 2022

This is interesting. If I remember right, I had at some point set the type hint to Optional[...], but then I noted that, even if it wasn't explicitly marked in this way but had a default value of None, the generated documentation correctly annotated the argument as optional. Then I removed the Optional. Edit: I left Optional only for optional return values.

Does this affect type checking with some tool, e.g. mypy?

@victorcwai
Copy link
Contributor Author

Yes, that affects type checking with pyre. But it seems that in mypy, initializing arguments to None is passable (under non-strict setting).

@mmckerns mmckerns self-requested a review August 7, 2022 16:54
@mmckerns
Copy link
Member

mmckerns commented Aug 7, 2022

@victorcwai: Do you experience the same issue with dump_module:

module: Union[ModuleType, str] = None,

and in load_module_asdict:
update: bool = False,

?

Copy link
Member

@mmckerns mmckerns left a comment

Choose a reason for hiding this comment

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

Please respond to the question from the reviewer. Otherwise LGTM.

@mmckerns mmckerns added this to the dill-0.3.6 milestone Aug 7, 2022
@victorcwai
Copy link
Contributor Author

victorcwai commented Aug 8, 2022 via email

@mmckerns
Copy link
Member

mmckerns commented Aug 8, 2022

Ok, makes sense. Thanks.

@victorcwai
Copy link
Contributor Author

Shall we merge this? :)

Copy link
Member

@mmckerns mmckerns left a comment

Choose a reason for hiding this comment

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

Please make a similar change to

module: Union[ModuleType, str] = None,
. Otherwise LGTM.

mmckerns added a commit that referenced this pull request Aug 13, 2022
@mmckerns mmckerns merged commit 25a7e45 into uqfoundation:master Aug 13, 2022
@mmckerns
Copy link
Member

I just went ahead and made the changes requested, then merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants