Messaging API client function with many possible parameters

Question

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 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,
        'user': user,  # can be a user or group key
        'message': message
    }

    # Support for non-required parameters of PushOver
    if 'title' in kwargs:
        data_out['title'] = kwargs['title']
    if 'device' in kwargs:
        temp = kwargs['device']
        if type(temp) == list:
            data_out['device'] = ','.join(temp)
        else:
            data_out['device'] = temp
        data_out['device'] = kwargs['device']
    if 'url' in kwargs:
        data_out['url'] = kwargs['url']
    if 'url_title' in kwargs:
        data_out['url_title'] = kwargs['url_title']
    if 'priority' in kwargs:
        data_out['priority'] = kwargs['priority']

        # Emergency prioritized messages require 'retry' and 'expire' to be defined
        if data_out['priority'] == PRIORITIES.EMERGENCY:
            if 'retry' not in kwargs:
                raise TypeError('Missing `retry` argument required for message priority of Emergency')
            else:
                retry_val = kwargs['retry']

                # 'retry' val must be a minimum of _MIN_RETRY and max of _MAX_EXPIRE
                if not (_MIN_RETRY <= retry_val <= _MAX_EXPIRE):
                    raise ValueError('`retry` argument must be at a minimum of {} and a maximum of {}'.format(
                        _MIN_RETRY, _MAX_EXPIRE
                    ))

                data_out['retry'] = retry_val
            if 'expire' not in kwargs:
                raise TypeError('Missing `expire` arguemnt required for message priority of Emergency')
            else:
                expire_val = kwargs['expire']

                # 'expire' val must be a minimum of _MIN_RETRY and max of _MAX_EXPIRE
                if not(_MIN_RETRY <= expire_val <= _MAX_EXPIRE):
                    raise ValueError('`expire` argument must be at a minimum of {} and a maximum of {}'.format(
                        _MIN_RETRY, _MAX_EXPIRE
                    ))

                data_out['expire'] = expire_val

            # Optionally a callback url may be supplied for the Emergency Message
            if 'callback' in kwargs:
                data_out['callback'] = kwargs['callback']

    if 'timestamp' in kwargs:
        data_out['timestamp'] = int(time.mktime(kwargs['timestamp'].timetuple()))
    if 'sound' in kwargs:
        data_out['sound'] = kwargs['sound']
    if 'html' in kwargs:
        data_out['html'] = int(kwargs['html'])

    return send(_push_url, data_out=data_out)

send from py_pushover/py_pushover/_base.py

def send(url, data_out=None, get_method=False):
    """
    Sends a request to the selected url with the payload `data_out`.  Set `get_method` to True to send as a GET request.
    Default request is a POST.
    :param str url: url to send the request to
    :param dict data_out: payload data to send
    :param bool get_method: True = GET request; False = POST request (default)
    :return dict: a dictionary with the json results of the request.
    """
    if get_method:
        res = requests.get(url, params=data_out)
    else:
        res = requests.post(url, params=data_out)

    res.raise_for_status()

    ret_dict = res.json()
    if 'X-Limit-App-Limit' in res.headers:
        ret_dict['app_limit'] = res.headers['X-Limit-App-Limit']
    if 'X-Limit-App-Remaining' in res.headers:
        ret_dict['app_remaining'] = res.headers['X-Limit-App-Remaining']
    if 'X-Limit-App-Reset' in res.headers:
        ret_dict['app_reset'] = res.headers['X-Limit-App-Reset']

    return ret_dict

Note: In order for this code to run properly you'll need to supply an app token and user/group id which is obtained from the Pushover site. Creating an application is free, but registering a device to send a message to isn't. There is, however, a free trial period of 7 days.

Answer 1

Whenever seeing repeated constructions I feel the urge to refactor it into 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 :-)}

Answer 2

You're using type(data_out.get('device')) == list, but it's actually recommended that you don't use this approach because it isn't reliable. It only tests against direct type identity. That means that it wouldn't work for subclasses.

Here's a simple example showing Counter, which inherits from dictionary:

>>> from collections import Counter
>>> counter = Counter()
>>> type(counter) == dict
False
>>> isinstance(counter, dict)
True

What's happening here is that Python sees that counter's type is Counter, not dict. However Counter inherits from dict, so counter is an instance of dict, meaning that the latter does evaluate as true.

Usually inheritance is fine for your purposes as classes should inherit most of the same functionality. Counters can still have values assigned and accessed the same way a dict can.

So in your case it's likely that your code will run regardless of whether the value is actually a list or just something that inherits from it.