Skip to content

Minor type fix#535

Merged
mmckerns merged 1 commit intouqfoundation:masterfrom
vcwai:patch-1
Aug 13, 2022
Merged

Minor type fix#535
mmckerns merged 1 commit intouqfoundation:masterfrom
vcwai:patch-1

Conversation

@vcwai
Copy link

@vcwai vcwai commented Aug 6, 2022

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

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

leogama commented Aug 6, 2022 •
edited
Loading

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?

@vcwai
Copy link
Author

vcwai commented Aug 7, 2022

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:

dill/dill/session.py

Line 132 in f196e3c

module: Union[ModuleType, str] = None,

and in load_module_asdict:

dill/dill/session.py

Line 494 in f196e3c

update: bool = False,

?

mmckerns
mmckerns reviewed Aug 7, 2022
View reviewed changes
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
@vcwai
Copy link
Author

vcwai commented Aug 8, 2022 via email

@mmckerns
Copy link
Member

mmckerns commented Aug 8, 2022

Ok, makes sense. Thanks.

@vcwai
Copy link
Author

vcwai commented Aug 13, 2022

Shall we merge this? :)

mmckerns
mmckerns requested changes Aug 13, 2022
View reviewed changes
Copy link
Member

@mmckerns mmckerns left a comment •
edited
Loading

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

dill/dill/session.py

Line 132 in f196e3c

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

mmckerns added a commit that referenced this pull request Aug 13, 2022
@mmckerns
patch to dump_module as in #535
9c68440
@mmckerns mmckerns merged commit 25a7e45 into uqfoundation:master Aug 13, 2022
@mmckerns
Copy link
Member

mmckerns commented Aug 13, 2022

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

Reviewers

@mmckerns mmckerns mmckerns requested changes

Assignees

No one assigned

Labels

compatibility

Projects

None yet

Milestone

dill-0.3.6

Development

Successfully merging this pull request may close these issues.

3 participants

@vcwai @leogama @mmckerns