Csdrhrt

I want this method to be completely understandable just from looking at the code and comments only.

def add_error(error_dict, key, err):

    """Given an error message, or a list of error messages, this method 

    adds it/them to a dictionary of errors.



    Doctests:

        >>> add_error({}, 'key1', 'error1')

        {'key1': ['error1']}

        >>> add_error({'key1': ['error1']}, 'key1', 'error2')

        {'key1': ['error1', 'error2']}

        >>> add_error({'key1': ['error1', 'error2']}, 'key2', 'error1')

        {'key1': ['error1', 'error2'], 'key2': ['error1']}

        >>> add_error({}, 'key1', ['error1', 'error2'])

        {'key1': ['error1', 'error2']}

        >>> add_error({}, 'key1', )

        {}

        >>> add_error({'key1': ['error1']}, 'key2', ['error1', 'error2'])

        {'key1': ['error1'], 'key2': ['error1', 'error2']}

        >>> add_error({}, 'key1', 23) # doctest: +IGNORE_EXCEPTION_DETAIL

        Traceback (most recent call last):

            ...

        TypeError: The error(s) must be a string, or a list of strings.

        >>> add_error({}, 'key1', [23]) # doctest: +IGNORE_EXCEPTION_DETAIL

        Traceback (most recent call last):

            ...

        TypeError: The error(s) must be a string, or a list of strings.

        >>> add_error({}, 'key1', ['error1', 23]) # doctest: 

        +IGNORE_EXCEPTION_DETAIL

        Traceback (most recent call last):

            ...

        TypeError: The error(s) must be a string, or a list of strings.

    """

    if not isinstance(err, list):

        err = [err]



    if not key in error_dict and len(err) > 0:

        error_dict[key] = 



    for e in err:

        if not isinstance(e, string_types):

            raise TypeError(

                'The error(s) must be a string, or a list of strings.'

            )

        error_dict[key].append(e)

    return error_dict

Hopefully, the code along with the comment does the job well, but I would still appreciate review(s) of this method. One thing I always keep on wondering is whether this is too many doc-tests for such a simple method. Thanks.

Consider narrowing accepted types

This might not be possible based on the context of your code, but if it is: arguments sharing one of many different types hinders and complicates testability and maintainability. There are many different solutions to this that will help this situation; one is accepting variadic arguments - i.e.

def add_error(error_dict, key, *errs):

This is still easily invocable without needing to wrap a single error in a list.

Use x not in instead of not x in

i.e.

if key not in error_dict

Lose your loop

and also lose your empty-list assignment, instead doing

error_dict.setdefault(key, ).extend(err)

If you use the variadic suggestion above, your entire function becomes that one line.

Immutable or not?

Currently you do two things - alter a dictionary and return it - when you should only pick one. Either make a copy of the dict and return an altered version, or modify the dict and don't return anything.

congratulations on writing a fairly clear, readable function! (And welcome!)

What types do you take?

You explicitly check for an instance of type list. I think you should invert your check, and look for a string type instead. The reason is that it would enable you to accept iterables other than list as your errors.

For example, you would be able to do something like:

add_error(edict, 'key', (str(e) for e in ...))

That last parameter is not a list, but it is something you might want to do. Also, *args is not a list but a tuple - you might want to splat a tuple rather than converting it to a list first.

What types do you take?

Your key parameter is always tested as a string. But dicts can have other key-types than string, and you neither test those, nor do you appear to have coded any kind of rejection on that basis. I suggest you add some tests that demonstrate your intent: is it okay to use non-strings as keys, or not?

What constraints exist on the errors?

I don't see any indication of what happens when duplicate errors are added. Is this intended to be allowed, or not?

What constraints exist on the keys?

Is it okay to use None as a key? How about '' (empty string)? Tests, please.

Type Hints

Based on your doctests, you must be using Python 3.6 or later (due to reliance on dictionary key order).

Since Python 3.5+ includes support for type hints, you could declare you function as:

def add_error(error_dict: dict, key: str, err: list) -> dict:

Or

from typing import List, Dict



def add_error(error_dict: Dict[str, List[str]], key: str,

                         err: List[str]) -> Dict[str]:

Of course, modify the type hints if you take the argument changes suggested in other answers. In particular, as is, the type of err is actually a variant type. I’d prefer a variable list of strings, *err: str.

Detect Errors before Modifying

If an error is not a string, you will raise an exception. But first, if the key does not exist in the dictionary, you add an empty list for that key.

If the error list contains strings before a non-string, you add those strings to the key’s list, then raise an exception part way through.

Consider moving the checks up to the start of the function, before any changes have been made.

if any(not isinstance(e, string_types) for e in err):

    raise TypeError("The error(s) must be a string, or list of strings")

Duck Typing

Why must the errors be a string? Any object can be converted to a string...