inveniosoftware / dojson

Simple pythonic JSON to JSON converter.
https://dojson.readthedocs.io
Other
10 stars 29 forks source link

utils: force_list returns a tuple #154

Closed kaplun closed 8 years ago

kaplun commented 8 years ago

utils.force_list(), despite its name and docstring, actually returns a tuple or None.

A better implementation could be:


def force_list(data):
    """Wrap data in list."""
    if data is None:
        return []
    elif not isinstance(data, (list, tuple, set)):
        return [data]
    elif isinstance(data, (tuple, set)):
        return list(data)
    return data

however then it would be necessary to amend filter_values to also filter away empty lists.

jirikuncar commented 8 years ago

IMHO it should not be necessary to make list from tuple and set. Can you provide a code snippet where the current code is causing troubles?

kaplun commented 8 years ago

It's not really trouble, but more a semantic issue where the name of the function doesn't correspond to its behavior.

In particular, see : https://github.com/inspirehep/inspire-next/pull/1070/files#diff-2dffa3ace57cdc6c5dabaa1d20b1fe6cR87

kaplun commented 8 years ago

As reported in https://github.com/inveniosoftware/dojson/pull/155#discussion_r70614422 finally the best solution is just to amend the docstring.

blixhavn commented 8 years ago

What's the conclusion on this? Do we make a PR to amend the docstring?

kaplun commented 8 years ago

In INSPIRE we have implemented a util called force_force_list(): https://github.com/inspirehep/inspire-next/blob/master/inspirehep/utils/helpers.py#L68

So IMHO it's OK to go either way.