Minor type fix

[victorcwai]

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

[leogama]

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]

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

[mmckerns]

@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]

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

[victorcwai]

Same issue with dump_module. But no problems with load_module_asdict.

…

On Sun, 7 Aug 2022, 7:08 pm Mike McKerns, ***@***.***> wrote: @victorcwai <https://github.com/victorcwai>: Do you experience the same issue with dump_module: https://github.com/uqfoundation/dill/blob/f196e3c9cbbba3326064c09760f7129df7f3de4d/dill/session.py#L132 and in load_module_asdict: https://github.com/uqfoundation/dill/blob/f196e3c9cbbba3326064c09760f7129df7f3de4d/dill/session.py#L494 ? — Reply to this email directly, view it on GitHub <#535 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACHZQIL7WUWKX2AMC2NPLCDVX7USFANCNFSM55ZK54FA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mmckerns]

Ok, makes sense. Thanks.

[victorcwai]

Shall we merge this? :)

[mmckerns]

Please make a similar change to

dill/dill/session.py

Line 132 in f196e3c

module: Union[ModuleType, str] = None,

. Otherwise LGTM.

[mmckerns]

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