-
Notifications
You must be signed in to change notification settings - Fork 76
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
Fix inconsistent access to sub-parameters #769
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
20103af
Test extension's parameter access
sandcha c7c9669
Clean whitespaces
sandcha 21bcb31
Test extension's parameter access for a given period
sandcha 1393b3d
fixup! Test extension's parameter access for a given period
sandcha 13e01e7
Uniformize adding parameter child nodes
Morendil a39951e
Provide a protocol for adding chilren in ParameterNodeAtInstant
Morendil 35ff5b2
Simplify tests
pblayo 13cf449
Bump version number
pblayo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
path.to.parameter
syntax works consistently across country and extensionOn 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Never mind, I hadn't seen the fixup PR.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.