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

678 select #683

Merged
merged 3 commits into from
Apr 16, 2021
Merged

678 select #683

merged 3 commits into from
Apr 16, 2021

Conversation

omry
Copy link
Owner

@omry omry commented Apr 16, 2021

Followup to discussion 678. easier to review individual commits.

  1. eliminating redundant tests.
  2. Change exception raised when trying to drill into a non-container with select from ConfigKeyError to ConfigTypeError.
  3. Change OmegaConf.select to return None if user attempts to drill into a non existing node (into a ValueNode or into None).

Closes #678

@omry omry requested review from odelalleau and Jasha10 April 16, 2021 02:38
@omry omry added this to the OmegaConf 2.1 milestone Apr 16, 2021
Copy link
Collaborator

@odelalleau odelalleau left a comment

Choose a reason for hiding this comment

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

Besides the comment below:

  • News item?
  • Maybe also consider fixing the case with a None container in this PR: {"x": "${y.z}", "y": DictConfig(None)} (ok too to leave it for another PR if you prefer)

tests/test_errors.py Show resolved Hide resolved
omegaconf/base.py Show resolved Hide resolved
@omry
Copy link
Owner Author

omry commented Apr 16, 2021

Besides the comment below:

  • News item?
  • Maybe also consider fixing the case with a None container in this PR: {"x": "${y.z}", "y": DictConfig(None)} (ok too to leave it for another PR if you prefer)
  1. Added news item.
  2. Do you mean:
OmegaConf.create({"x": "${y.z}", "y": DictConfig(None)}).x

Raising the wrong exception?
this seems to be an existing (low priority) bug that is not related to this one.

File a separate issue?

@odelalleau
Copy link
Collaborator

File a separate issue?

New issue: #685

Copy link
Collaborator

@odelalleau odelalleau left a comment

Choose a reason for hiding this comment

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

Good to merge after fixing this small typo!

news/678.bugfix Outdated Show resolved Hide resolved
@omry
Copy link
Owner Author

omry commented Apr 16, 2021

File a separate issue?

New issue: #685

Actually not the same issue I was thinking about.
Run this:

OmegaConf.create({"x": "${y.z}", "y": DictConfig(None)}).x

no OmegaConf.select involved.

@omry omry merged commit fc1a560 into master Apr 16, 2021
@omry omry deleted the 678-select branch April 16, 2021 20:22
@odelalleau
Copy link
Collaborator

File a separate issue?

New issue: #685

Actually not the same issue I was thinking about.
Run this:

OmegaConf.create({"x": "${y.z}", "y": DictConfig(None)}).x

no OmegaConf.select involved.

Hopefully both can be fixed at the same time :)

@omry
Copy link
Owner Author

omry commented Apr 16, 2021

Looks like the fix accidentally fixed the other bug:

OmegaConf.create({"x": "${y.z}", "y": DictConfig(None)}).x
...
InterpolationKeyError: Interpolation key 'y.z' not found
    full_key: x
    object_type=dict

@odelalleau
Copy link
Collaborator

Looks like the fix accidentally fixed the other bug:

Nice -- that's what I was hoping for since both were raising the same exception.

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.

OmegaConf.select raises an exception when selecting an invalid key in some cases
2 participants