Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

antoni-szych-rtbhouse
Copy link
Contributor

@antoni-szych-rtbhouse antoni-szych-rtbhouse commented Dec 11, 2023

This is to address an issue introduced in #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:

  • integers, as used in list indices
  • actually, any hashable type that could be a dictionary key (however as this is not my usecase and I doubt it's widely needed, I didn't try to introduce it here EDIT: after discussion I've used typing.Hashable in the last commit. In case Alec will decide that only strings and ints are sufficient, he can just skip the last commit.)

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
Copy link
Contributor

ds-cbo commented Dec 12, 2023

Answering here for clarity:

Hi @ds-cbo, I've updated the typings to allow integers in paths in #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
Copy link
Contributor Author

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.

@antoni-szych-rtbhouse antoni-szych-rtbhouse changed the title fix: allow path to be a list of strings as well as integers fix: allow path to be a list of strings, integers or any other hashables Dec 12, 2023
@alecthomas
Copy link
Owner

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

@antoni-szych-rtbhouse
Copy link
Contributor Author

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

Of course, I've added a few.

@alecthomas alecthomas merged commit 264b7e4 into alecthomas:master Dec 13, 2023
5 checks passed
@alecthomas
Copy link
Owner

Thanks!

@antoni-szych-rtbhouse antoni-szych-rtbhouse deleted the path-including-ints branch December 28, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants