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

Error reporting is limited when dict key is unhashable #17

Closed
zaneb opened this issue Mar 28, 2019 · 5 comments · Fixed by #18
Closed

Error reporting is limited when dict key is unhashable #17

zaneb opened this issue Mar 28, 2019 · 5 comments · Fixed by #18

Comments

@zaneb
Copy link

zaneb commented Mar 28, 2019

If you attempt to parse code that contains a mapping with an unhashable key, yaml raises a ConstructorError that tells the user exactly where the issue was. However, oyaml raises a TypeError, which may be missed by exception handling code expecting a ConstructorError, and which contains no information about where the problem occurred.

>>> def test():
...   try:
...     yaml.safe_load("{foo: bar}: baz")
...   except Exception as e:
...     print("%s: %s" % (type(e).__name__, e))
... 
>>> import yaml
>>> test()
ConstructorError: while constructing a mapping
found unacceptable key (unhashable type: 'dict')
  in "<string>", line 1, column 1:
    {foo: bar}: baz
    ^
>>> import oyaml
>>> test()
TypeError: unhashable type: 'OrderedDict'
@wimglenn
Copy link
Owner

wimglenn commented Mar 28, 2019

What version(s)? Can not reproduce with oyaml v0.8 / PyYAML 5.1 / Python 3.7.2 (linux). I get the same richer exception context for both pyyaml and oyaml.

Edit: I can reproduce it on 2.7

@zaneb
Copy link
Author

zaneb commented Mar 28, 2019

Anything before 3.6 I expect. oyaml doesn't patch the Loader after that.

@wimglenn
Copy link
Owner

wimglenn commented Mar 28, 2019

Hmm, it's not really amenable to monkeypatching here since the mapping type is just a literal in the local scope. I really need this to be more like def construct_mapping(self, node, deep=False, factory=dict):. But I'll see what I can do. Thanks for the report!

@zaneb
Copy link
Author

zaneb commented Mar 28, 2019

Yes, it's a pain. You have to choose between inefficiency and reimplementing a bunch of internal logic :(

@wimglenn
Copy link
Owner

wimglenn commented Mar 28, 2019

There could also be an ast rewrite 😈
Anyway, the try/re-raise in #18 seems an acceptable solution, time will tell if travis agrees ..

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 a pull request may close this issue.

2 participants