Fix inconsistent access to sub-parameters #769
Conversation
Morendil
force-pushed
the
extension-parameters
branch
from
0635c47 to
701e9b1
Compare
tests/core/test_extensions.py
Outdated
| def test_load_extension(): | ||
| country_template_variables_number = 16 |
There was a problem hiding this comment.
This is an extremely fragile test! As soon as we'll add a new variables in the CT, it will break...
tests/core/test_extensions.py
Outdated
| tbs = CountryTaxBenefitSystem() | ||
|
|
||
|
|
||
| def test_extension_not_already_loaded(): | ||
| assert tbs.get_variable('local_town_child_allowance') is None | ||
|
|
||
|
|
||
| def walk_and_count(node): |
There was a problem hiding this comment.
We can now use the new helper to avoid manually doing that: http://openfisca.org/doc/openfisca-python-api/parameters.html#openfisca_core.parameters.ParameterNode.get_descendants
tests/core/test_extensions.py
Outdated
| assert walk_and_count(tbs.parameters) == country_template_parameters_number + 1 | ||
| assert tbs.parameters('2016-01').local_town.child_allowance.amount == 100.0 | ||
| assert tbs.parameters.local_town.child_allowance.amount('2016-01') == 100.0 | ||
| assert tbs.parameters.local_town.child_allowance.amount is not None |
There was a problem hiding this comment.
There are 11 assertions in this test. This is a lot. Besides, many of them are not really related to extensions.
I understand these assertions were useful to explicit the issue we were encountering, but I don't think they all deserve to be permanently added to the codebase.
pblayo
force-pushed
the
extension-parameters
branch
from
0072ed2 to
35ff5b2
Compare
pblayo
dismissed
fpagnoux’s stale review
Changes request have been taken into account
|
On a meta note, we usually pretty rarely dismiss reviews unless:
|
| ### 24.9.5 [#771](https://github.com/openfisca/openfisca-core/pull/771) | ||
| ### 24.9.7 [#769](https://github.com/openfisca/openfisca-core/pull/769) | ||
|
|
||
| - Unify the protocol for appending sub-parameters |
There was a problem hiding this comment.
Not sure this is limpid for a user of the OpenFisca-Core library.
What does mean "appending sub-parameters" from them? Is there a protocol for that?
From the user point of view, I think what has been done here is:
- Fix an inconsistency in parameter navigation
- In some cases, for instance while using extensions, the `parameters.path.to.subparams` notation was not working
On a meta note, I think this is another reason why the Changelog should ideally written before the review, so that it can be improve, as it's a pretty important part of each PR.
There was a problem hiding this comment.
Yes, too implementation-centric, thanks for the suggestion; I can pick it up on a later PR. I didn't much like "fix" as a verb though, it's too generic, and that's part of why I looked for more specific terms. How about
- Ensure
path.to.parametersyntax works consistently across country and extension
On the meta level: fixing unnecessary merge conflicts is one of the least appealing parts of the merge process at the moment, so much I'd make the diametrically opposed policy proposal, i.e. delay the CHANGELOG.md/setup.py until just before merge.
As a "best of both worlds" proposal, I suggest improving the description collaboratively by editing the PR's description at review time. Then creating the CHANGELOG entry is a matter of copy/paste (until we automate it away).
There was a problem hiding this comment.
(Never mind, I hadn't seen the fixup PR.)
There was a problem hiding this comment.
We can include the fixup PR content in another PR to come (especially as right now editing the changelog triggers the version bump requirement), and feel free to edit the text it if you like.
Fixes #762
Bug fixes
parameters.py, there is rampant duplication of the following two lines:However, in a few cases - including merging parameters from extensions, as in the issue referenced - the array is written to, without the call to setattr taking place. This will result in an inconsistent state and a ParameterNotFound exception even though the parameter seems to have been correctly loaded. This PR aims to channel all additions of a descendant to a parameter node via add_child.