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

New auxiliary function to get value for options: _getopt() #555

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leogama
Copy link
Contributor

@leogama leogama commented Oct 4, 2022

From #475

@mmckerns mmckerns self-requested a review October 4, 2022 10:16
@mmckerns mmckerns added this to the dill-0.3.7 milestone Oct 23, 2022
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.

I'm not sure if this is a good idea in its current form.

if arg is not None:
return arg
else:
return settings[key]
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit unorthodox that the alternate is given as the first two values, and then the arg/kwd you are primarily interested in is given after that. Consider the syntax of getattr and similar.

So, this function pops key out of kwds (isn't get a better choice?), and if key is found and not None, then it is returned. If not found, then use arg if arg is provided, unless it's None... then default back to settings[key]. It's also expected that arg and kwds are never both given... so essentially, you have two one-liners in a single function, where any interaction between the two one-liners is an error. I'm not seeing the utility of this function, in that case. I could see using it, maybe, if you wanted to hide the import of settings within the function -- so _getopt(key, arg=None, *, kwds=None).

self._byref = _getopt(settings, 'byref', kwds=kwds)
"""
# Sanity check, it's a bug in calling code if False.
assert kwds is None or arg is None
Copy link
Member

Choose a reason for hiding this comment

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

To me, an AssertionError is not expected... shouldn't it be a ValueError?

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 review comments.

@mmckerns mmckerns modified the milestones: dill-0.3.7, dill-0.3.8 Mar 21, 2023
@mmckerns mmckerns modified the milestones: dill-0.3.8, dill-0.3.9 Nov 14, 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.

None yet

2 participants