To container throw on missing

[Jasha10]

Closes #501

[omry]

It will probably be simpler to
node = conf._get_node(key, throw_on_missing=True)

Try to line the error message with the normal error for missing values (raise MissingMandatoryValue("Missing mandatory value: $FULL_KEY").

[Jasha10]

Rebased onto master.

[Jasha10]

It will probably be simpler to
node = conf._get_node(key, throw_on_missing=True)

conf._get_node does not have a throw_on_missing keyword option.
Do you mean that I should add a throw_on_missing keyword argument to the _get_node signature?

[omry]

Do you mean that I should add a throw_on_missing keyword argument to the _get_node signature?

This is what I had in mind, yes. I added it in #513.

[ghost]

Congratulations 🎉. DeepCode analyzed your code in 2.618 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

[omry]

Marking as request-changes (see prev comments)

[Jasha10]

Rebased onto master

[omry]

Can't comment on code you didn't change but I found what I was thinking about with the _get_node() comment:

            retdict: Dict[str, Any] = {}
            for key in conf.keys():
                node = conf._get_node(key)

In above _get_node(), what happens if you use conf._get_node(key, throw_on_missing=throw_on_missing)?

(You will probably need to do the same for ListConfig somwehere).

[Jasha10]

In above _get_node(), what happens if you use conf._get_node(key, throw_on_missing=throw_on_missing)?

That works!
There are a few test failures in test_errors.py which look like this:

AssertionError: Regex pattern 'Missing mandatory value: subcfg.x' does not match 'Missing mandatory value'.

If it's ok with you to change the error message, I'll make the change to using conf._get_node(key, throw_on_missing_value=throw_on_missing) and will update the test_errors.py error messages.

Edit: nevermind, all of the assertion errors were for test cases that are introduced by this PR; changing the error messages will not break anything. I'll make the change to using conf._get_node(key, throw_on_missing_value=throw_on_missing)

[omry]

much cleaner.

[Jasha10]

Currently code coverage tests are failing because the except blocks from the latest commit fe32082 have no coverage. Those try/except statements were added in anticipation of #545, and the coverage issue should be resolved once this branch is rebased onto that one.

[omry]

1. Why is _dereference_node throwing a MandatoryMissingValue is throw_on_missing is False?
2. Why are you passing False to throw_on_missing and not the input flag?

[Jasha10]

1. Why is _dereference_node throwing a MandatoryMissingValue is throw_on_missing is False?

This is because we are calling _dereference_node with throw_on_resolution_failure=True.

Edit:
I was mistaken about what the throw_on_resolution_failure flag is doing.
I think I need to investigate this a bit more...

If #545 is merged in it's current state, _dereference_node will raise MandatoryMissingValue if throw_on_resolution_failure==True and the interpolation cannot be resolved.

This being said, I actually think it makes more sense to change this from

_dereference_node(..., throw_on_resolution_failure=True)

to

_dereference_node(..., throw_on_resolution_failure=throw_on_missing)

so that resolution failure will not cause error if the user does not specify the throw_on_missing option. I'll add tests to this effect...

[Jasha10]

2. Why are you passing False to throw_on_missing and not the input flag?

The current workflow is:

• first, get the node and (optionally) throw if missing:

node = conf._get_node(key, throw_on_missing_value=throw_on_missing)

• second: if resolve==True, dereference the node:

node = node._dereference_node(throw_on_missing=False, throw_on_resolution_failure=True)

Since the _get_node call already has throw_on_missing_value=throw_on_missing, it would be redundant to use the throw_on_missing flag in the _dereference_node call as well.

[Jasha10]

#545 will change the behavior of _dereference_node significantly enough that it makes sense to put this PR on hold until #545 gets merged.

[Jasha10]

Marking this PR as needing possible API redesign after #502 gets merged.

[Jasha10]

This PR is old; closing in favor of #730.