Fix handling of `throw_on_missing` flag for `select()` on ListConfig #564

odelalleau · 2021-02-24T13:24:11Z

Fixes #563

omry · 2021-02-24T18:33:31Z

news/563.bugfix

@@ -0,0 +1 @@
+Selecting a missing node from a ListConfig with `throw_on_missing` set to True now raises the intended exception.


Function name here to be more specific about what you mean by "selecting"

Suggested change

Selecting a missing node from a ListConfig with `throw_on_missing` set to True now raises the intended exception.

OmegaConf.select() a missing node from a ListConfig with `throw_on_missing` set to True now raises the intended exception.

Done in b9a5db6

omry · 2021-02-24T18:36:43Z

tests/test_select.py

@@ -85,7 +86,7 @@ def test_select_default(
        pytest.param({"missing": "???"}, "missing", id="missing"),
    ],
 )
-def test_select_default_throw_on_missing(
+def test_select_default_throw_on_missing_dict(


old function name seems better. why rename it?

Before there was just the test_select_default_throw_on_missing function;
now there is a test_select_default_throw_on_missing_dict function and a test_select_default_throw_on_missing_list function.

this function is testing both dicts and lists.

It was only testing a dict here, but actually you're right that I can re-use the same test for lists as well (somehow I thought that set_struct was only for DictConfig, but looks like I was wrong).

I changed it in ~~cef0a23~~ 585183c

omry · 2021-02-24T18:37:35Z

tests/test_select.py

+@pytest.mark.parametrize(
+    "cfg, key",
+    [
+        pytest.param(["???"], "0", id="missing"),
+    ],
+)
+def test_select_default_throw_on_missing_list(
+    cfg: Any,
+    key: Any,
+) -> None:
+    cfg = OmegaConf.create(cfg)
+
+    # throw on missing still throws if default is provided
+    with pytest.raises(MissingMandatoryValue):
+        OmegaConf.select(cfg, key, default=0, throw_on_missing=True)
+
+


looks like this is covered by the first test you changed?

I think that this test covers the throw_on_missing=True case, the other one does not.

No, the list:missing test I added is a success case when throw_on_missing is False (it was working before as well, but it wasn't tested, I just added it for better test coverage)

How is it a success case if the test body has this?

with pytest.raises(MissingMandatoryValue): ...

Trying to clear up the confusion:

I thought by "first test you changed" you meant test_select(): I indeed added a new test here (id list:missing) to test the case where we select a missing value with throw_on_missing set to False (which worked before, and still works here)

The test with with pytest.raises(MissingMandatoryValue): is test_select_default_throw_on_missing: this tests the case where throw_on_missing is True. The new test I added here would have failed before this PR.

Jasha10

Looks good.

omry · 2021-02-24T21:08:03Z

tests/test_select.py

@@ -85,7 +86,7 @@ def test_select_default(
        pytest.param({"missing": "???"}, "missing", id="missing"),
    ],
 )
-def test_select_default_throw_on_missing(
+def test_select_default_throw_on_missing_dict(


this function is testing both dicts and lists.

omry · 2021-02-24T21:12:47Z

tests/test_select.py

+@pytest.mark.parametrize(
+    "cfg, key",
+    [
+        pytest.param(["???"], "0", id="missing"),
+    ],
+)
+def test_select_default_throw_on_missing_list(
+    cfg: Any,
+    key: Any,
+) -> None:
+    cfg = OmegaConf.create(cfg)
+
+    # throw on missing still throws if default is provided
+    with pytest.raises(MissingMandatoryValue):
+        OmegaConf.select(cfg, key, default=0, throw_on_missing=True)
+
+


How is it a success case if the test body has this?

with pytest.raises(MissingMandatoryValue): ...

omry

Looking good.
I think GitHub diff view played some tricks on all of us here.

odelalleau added 2 commits February 24, 2021 08:42

Fix handling of throw_on_missing flag for select() on ListConfig

cc90bb3

Fixes omry#563

Add news fragment

f542d71

odelalleau force-pushed the list_select_throw_missing branch from 678fa4e to f542d71 Compare February 24, 2021 13:43

odelalleau mentioned this pull request Feb 24, 2021

Improve handling of interpolations pointing to missing nodes #545

Merged

omry requested changes Feb 24, 2021

View reviewed changes

Jasha10 approved these changes Feb 24, 2021

View reviewed changes

Improve wording of news fragment

b9a5db6

omry requested changes Feb 24, 2021

View reviewed changes

Combine two tests together

585183c

odelalleau force-pushed the list_select_throw_missing branch from cef0a23 to 585183c Compare February 24, 2021 22:21

omry approved these changes Feb 24, 2021

View reviewed changes

omry merged commit 01c530d into omry:master Feb 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of `throw_on_missing` flag for `select()` on ListConfig #564

Fix handling of `throw_on_missing` flag for `select()` on ListConfig #564

odelalleau commented Feb 24, 2021

omry Feb 24, 2021

odelalleau Feb 24, 2021

omry Feb 24, 2021

Jasha10 Feb 24, 2021

omry Feb 24, 2021

odelalleau Feb 24, 2021 •

edited

Loading

omry Feb 24, 2021

Jasha10 Feb 24, 2021

odelalleau Feb 24, 2021

omry Feb 24, 2021

odelalleau Feb 24, 2021

Jasha10 left a comment

omry Feb 24, 2021

omry Feb 24, 2021

omry left a comment

		@@ -0,0 +1 @@
		Selecting a missing node from a ListConfig with `throw_on_missing` set to True now raises the intended exception.

	Selecting a missing node from a ListConfig with `throw_on_missing` set to True now raises the intended exception.
	OmegaConf.select() a missing node from a ListConfig with `throw_on_missing` set to True now raises the intended exception.

Fix handling of throw_on_missing flag for select() on ListConfig #564

Fix handling of throw_on_missing flag for select() on ListConfig #564

Conversation

odelalleau commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odelalleau Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jasha10 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omry left a comment

Choose a reason for hiding this comment

Fix handling of `throw_on_missing` flag for `select()` on ListConfig #564

Fix handling of `throw_on_missing` flag for `select()` on ListConfig #564

odelalleau Feb 24, 2021 •

edited

Loading