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

More explicit error when trying to access the child of non-container node #444

Merged
merged 1 commit into from
Nov 27, 2020

Conversation

odelalleau
Copy link
Collaborator

When using ${a.b}, if a is not a container node, the previous code was failing with an obscure AssertionError.

This commit was originally the first commit in the stack in #321 but was unrelated to other changes in that PR, so it makes more sense to create a different PR.

I don't think it's worth adding an issue and a news item for this (since it seems pretty minor) but let me know if I should.

Note: the motivation for adding test_assertion_error() is that otherwise coverage would drop below 100%.

…node

When using ${a.b}, if `a` is not a container node, the previous code was
failing with an obscure AssertionError.
@omry
Copy link
Owner

omry commented Nov 27, 2020

curious about test_assertion_error.
what line in the code is not covered unless you have that?

Copy link
Owner

@omry omry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regardless, this looks fine.
I will merge after you answer my question.

@odelalleau
Copy link
Collaborator Author

curious about test_assertion_error.
what line in the code is not covered unless you have that?

This line:

@omry
Copy link
Owner

omry commented Nov 27, 2020

Okay, let's keep that - it may come in handy next time there is some random assertion.

@omry omry merged commit 6f06c89 into omry:master Nov 27, 2020
@omry
Copy link
Owner

omry commented Apr 14, 2021

Generally, the contact of OmegaConf.select is that it returns None if the field is not found.
This fix is causing inconsistent behavior:

In [14]: cfg
Out[14]: {'a': 10, 'b': None}

In [15]: print(OmegaConf.select(cfg, "foo.bar"))
None

In [16]: print(OmegaConf.select(cfg, "b.bar"))
...
ConfigKeyError: Error trying to access b.bar: node `b` is not a container and thus cannot contain `bar`
    full_key: b.bar
    object_type=dict

Obviously this is better than assertion error, but it's not the correct fix, at least for the purposes of the public OmegaConf.select().

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.

2 participants