patternpythonMinor
Messaging API client function with many possible parameters
Viewed 0 times
withfunctionmessagingclientpossiblemanyparametersapi
Problem
I'm working an open source project written in Python that is a wrapper to the Pushover API called py_pushover.
The main function of this wrapper is the
push_message from py_pushover/py_pushover/message.py
```
def push_message(token, user, message, **kwargs):
"""
Send message to selected user/group/device.
:param str token: application token
:param str user: user or group id to send the message to
:param str message: your message
:param str title: your message's title, otherwise your app's name is used
:param str device: your user's device name to send the message directly to that device
:param list device: your user's devices names to send the message directly to that device
:param str url: a supplementary URL to show with your message
:param str url_title: a title for your supplementary URL, otherwise just the URL is shown
:param int priority: message priority (Use the Priority class to select)
:param int retry: how often (in seconds) the Pushover servers will retry the notification to the user (required
only with priority level of Emergency)
:param int expire: how many seconds your notification will continue to be retried (required only with priority
level of Emergency)
:param datetime timestamp: a datetime object repr the timestamp of your message's date and time to display to the user
:param str sound: the name of the sound to override the user's default sound choice (Use the Sounds consts to
select)
:param bool html: Enable rendering message on user device using HTML
"""
data_out = {
'token': token,
'u
The main function of this wrapper is the
push_message function which allows the user to push a notification. The API has 10 possible parameters with only 3 being required. I've handled those three required with named parameters and the optional parameters using kwargs. Is this the most pythonic way of handling a large amount of parameters?push_message from py_pushover/py_pushover/message.py
```
def push_message(token, user, message, **kwargs):
"""
Send message to selected user/group/device.
:param str token: application token
:param str user: user or group id to send the message to
:param str message: your message
:param str title: your message's title, otherwise your app's name is used
:param str device: your user's device name to send the message directly to that device
:param list device: your user's devices names to send the message directly to that device
:param str url: a supplementary URL to show with your message
:param str url_title: a title for your supplementary URL, otherwise just the URL is shown
:param int priority: message priority (Use the Priority class to select)
:param int retry: how often (in seconds) the Pushover servers will retry the notification to the user (required
only with priority level of Emergency)
:param int expire: how many seconds your notification will continue to be retried (required only with priority
level of Emergency)
:param datetime timestamp: a datetime object repr the timestamp of your message's date and time to display to the user
:param str sound: the name of the sound to override the user's default sound choice (Use the Sounds consts to
select)
:param bool html: Enable rendering message on user device using HTML
"""
data_out = {
'token': token,
'u
Solution
Whenever seeing repeated constructions I feel the urge to refactor it into
This can be simplified to:
You could even extract the entire list into a constant outside of the function, and make it even neater.
Now that you've gathered all the data from
You'll get the gist. The main idea here is to first transfer all of the parameters, then validate them afterwards. By using
I would also do a similar change to the transformations in your
Other than this your coding style seems to match PEP8 nicely, and is OK to read and understand. If you want to make the previous
Disclaimer: Code is somewhat untested :-)
for loop, and in your case you have a lot of repeating stuff similar to:if 'title' in kwargs:
data_out['title'] = kwargs['title']This can be simplified to:
for parameter in ('title', 'device', 'url', 'url_title', 'priority',
'retry', 'expire', 'callback', 'timestamp',
'sound', 'html'):
if parameter in kwargs:
data_out[parameter] = kwargs[parameter]You could even extract the entire list into a constant outside of the function, and make it even neater.
Now that you've gathered all the data from
kwargs, you could continue doing the validation of the parameters. if type(data_out.get('device')) == list:
data_out['device'] = ','.join(data_out['device']
if data_out.get('priority') == PRIORITIES.EMERGENCY:
retry_val = data_out.get('retry')
if retry_val is None:
raise ValueError('Need retry value....')
else:
if not ( _MIN_RETRY <= retry_val <= _MAX_EXPIRE ):
raise ValueError('Retry not in range')
...You'll get the gist. The main idea here is to first transfer all of the parameters, then validate them afterwards. By using
get() we avoid doing the exception catching needed if the key doesn't exists, which could be useful in your case. I would also do a similar change to the transformations in your
send() function:HEADER_TO_DICT = {
'X-Limit-App-Limit' : 'app_limit',
'X-Limit-App-Remaining' : 'app_remaining'
'X-Limit-App-Reset' : 'app_reset'
}
# For Python 3, use HEADER_TO_DICT.items()
for header, dict_value in HEADER_TO_DICT.iteritems():
if header in res.headers:
ret_dict[dict_value] = res.headers[header]Other than this your coding style seems to match PEP8 nicely, and is OK to read and understand. If you want to make the previous
for loop independent of Python version, you should read "Iterating over dictionaries using for loops in Python", and if not happy with any of those solutions, go back to the following variant:for header in HEADER_TO_DICT:
if header in res.headers:
ret_dict[HEADER_TO_DICT[header]] = res.headers[header]Disclaimer: Code is somewhat untested :-)
Code Snippets
if 'title' in kwargs:
data_out['title'] = kwargs['title']for parameter in ('title', 'device', 'url', 'url_title', 'priority',
'retry', 'expire', 'callback', 'timestamp',
'sound', 'html'):
if parameter in kwargs:
data_out[parameter] = kwargs[parameter]if type(data_out.get('device')) == list:
data_out['device'] = ','.join(data_out['device']
if data_out.get('priority') == PRIORITIES.EMERGENCY:
retry_val = data_out.get('retry')
if retry_val is None:
raise ValueError('Need retry value....')
else:
if not ( _MIN_RETRY <= retry_val <= _MAX_EXPIRE ):
raise ValueError('Retry not in range')
...HEADER_TO_DICT = {
'X-Limit-App-Limit' : 'app_limit',
'X-Limit-App-Remaining' : 'app_remaining'
'X-Limit-App-Reset' : 'app_reset'
}
# For Python 3, use HEADER_TO_DICT.items()
for header, dict_value in HEADER_TO_DICT.iteritems():
if header in res.headers:
ret_dict[dict_value] = res.headers[header]for header in HEADER_TO_DICT:
if header in res.headers:
ret_dict[HEADER_TO_DICT[header]] = res.headers[header]Context
StackExchange Code Review Q#112751, answer score: 3
Revisions (0)
No revisions yet.