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

do not use kwargs in ExistDB.__init__ #1

Closed
lddubeau opened this issue May 20, 2016 · 5 comments
Closed

do not use kwargs in ExistDB.__init__ #1

lddubeau opened this issue May 20, 2016 · 5 comments

Comments

@lddubeau
Copy link
Contributor

As we speak the __init__ for ExistDB is defined like this:

    def __init__(self, server_url=None, username=None, password=None,
                 resultType=None, encoding='UTF-8', verbose=False,
                 keep_alive=None, **kwargs):

However, I do not see any reason for **kwargs there in the current code base. **kwargs is normally useful when a function accepts a series of parameters that it does not itself understand but that are going to be passed to another function. It allows passing the parameter payload to the other function opaquely, but the code of __init__ currently does not do this.

Conversely, using **kwargs has for consequence to hide possible errors in the parameter list. I had a case where I was doing ExistDB(usernname="whatever", ...) Note the typo in the parameter name. With **kwargs the typo is ignored. If **kwargs were replaced with timeout=None, the Python parser would immediately raise an error when an unsupported parameter is passed.

@rlskoeser
Copy link
Contributor

Thanks, good idea.

@rlskoeser
Copy link
Contributor

After looking at the code again, the reason I'm using **kwargs here is so I can differentiate between no timeout specified and timeout of None so that an explicit keyword argument can be used to override a configured timeout value in Django settings. I'm not certain if that functionality is actually used anywhere, but it does seem like it could be valuable in some cases. I'm open to suggestions for another approach that doesn't require using **kwargs, right now the only things occurring to me seem like hacks.

@lddubeau
Copy link
Contributor Author

Yes, I noticed that this was one use for kwargs in the current code but the same effect can be achieved without kwargs. You could create a special value earlier in the module:

DEFAULT_TIMEOUT = object()

and then have timeout=DEFAULT_TIMEOUT in the list of parameters for __init__. Then you have 2 broad cases in __init__:

  1. timeout is DEFAULT_TIMEOUT: the caller wants the default behavior (either because no value was specified or because the caller specified timeout=DEFAULT_TIMEOUT explicitly, which is redundant but could happen I guess): obtain the timeout from the Django settings. If such settings do not exist, then timeout becomes None.
  2. Otherwise, the user specifically wants to set a specific timeout value with the constructor: ignore all other sources for timeout.

rlskoeser added a commit that referenced this issue May 27, 2016
Move check for keep_alive in django settings into optional django
import block so it will work without django #3

Fixes #1
@rlskoeser
Copy link
Contributor

Thanks for this prompt to revisit the timeout handling, it turns out I wasn't actually passing the configured timeout through to the rest api requests! I hadn't previously figured out a way to test the timeout handling, but with this update it seemed much clearer how to test the different possible cases.

@rlskoeser
Copy link
Contributor

Fix will be in the next release

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

No branches or pull requests

2 participants