alecthomas / voluptuous

CONTRIBUTIONS ONLY: Voluptuous, despite the name, is a Python data validation library.
https://pypi.org/project/voluptuous
BSD 3-Clause "New" or "Revised" License
1.81k stars 219 forks source link

fix: allow path to be a list of strings, integers or any other hashables #497

Closed antoni-szych-rtbhouse closed 9 months ago

antoni-szych-rtbhouse commented 9 months ago

This is to address an issue introduced in https://github.com/alecthomas/voluptuous/pull/475

As I understand, in this PR, @ds-cbo assumed that the path fragments could only be strings. However, it just occured to me that there is no reason to not allow:

Just to confirm, I've checked the examples from the readme - it seems that they demonstrate exactly this - integers in path as list indices:

> import voluptuous
> schema=voluptuous.Schema([[2,3],6])
> try:
>   schema([[6]])
> except Exception as exc:
>   exc1=exc

> exc1
MultipleInvalid([ScalarInvalid('not a valid value')])

> str(exc1)
'not a valid value @ data[0][0]'

> exc1.path
[0, 0]

Also, this seems to be the intention behind the comment here:

def _nested_getitem(data: typing.Any, path: typing.List[typing.Union[str, int]]) -> typing.Optional[typing.Any]:
    for item_index in path:
        try:
            data = data[item_index]
        except (KeyError, IndexError, TypeError):
            # The index is not present in the dictionary, list or other
            # indexable or data is not subscriptable
            return None
ds-cbo commented 9 months ago

Answering here for clarity:

Hi @ds-cbo, I've updated the typings to allow integers in paths in https://github.com/alecthomas/voluptuous/pull/497 (useful to show that the validation failed for e.g. 5th element of some list), but wanted to confirm with you - was there any special reason you decided to allow only strings in path?

There was no special reason other than "I imagine that this is the most logical" but I can see why integers could also be useful. While you're at it, I would suggest using typing.Hashable directly, to prevent new PRs about the same issue. (As a bonus it's both shorter and clearer than typing.Union[str, int]])

antoni-szych-rtbhouse commented 9 months ago

While you're at it, I would suggest using typing.Hashable directly, to prevent new PRs about the same issue.

Sure thing, please take a look at the updated version.

alecthomas commented 9 months ago

Seems reasonable to me, can you add some tests for non-string keys please?

antoni-szych-rtbhouse commented 9 months ago

Seems reasonable to me, can you add some tests for non-string keys please?

Of course, I've added a few.

alecthomas commented 9 months ago

Thanks!