-
Notifications
You must be signed in to change notification settings - Fork 77
Handle nested dictionaries in config parameter storage #942
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
Handle nested dictionaries in config parameter storage #942
Conversation
|
This looks pretty good. I would it to be a bit more general, allowing an arbitrary level of nesting. I would go for a recursive approach. |
e7bc4b8 to
2573967
Compare
gonzaponte
left a comment
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.
Apologies for the late reply. I've had a hectic couple of weeks.
The proposed solution is excellent, I think. Just a minor comment: you either remove the dev argument (what does it stand for?) or pass it as an argument when you call the function from within the loop. I don't think we want to make it this generic, but both approaches are fine by me.
|
No need to apologise! You're absolutely right Gonzalo: the "dev" was meant to stand for "divider" (but I made a typo). There's no need for it to be an argument so I'll implement that and the rest of the changes now. |
|
Sorry, I thought I have already commented here. This is almost ready, let's rewrite the commit history following these recommendations. After that, I'll approve it. |
fcb857d to
f02f34f
Compare
|
Hi Gonzalo! Is the commit history okay now? |
|
The second commit says "Alter function & test...", but the changes are more like "Introduce function and alter test ...". I suggest you put the second version of the test (the one prior to "simplification") already in the first commit and then introduce the final version of the function in the second commit, leaving the third one as it is now. Let me know if you need help with this. |
|
I'll try this now, will let you know if I need help. Thanks! |
f02f34f to
7384771
Compare
|
I think this should be better now. I was not able to put the second version of the testing function in the first commit because I committed it at the same time as the "final version" of the unpacking function. For this reason, I thought they would be better in the second commit together. Let me know what you think of these changes |
a4b9990 to
ecb27c6
Compare
|
The history has been rewritten with peras to learn some magit. |
gonzaponte
left a comment
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.
Handles nested dictionaries elegantly in the config output. And it's tested. Nice job!
|
@Luismiguelvillar please merge! |

This PR fixes #941 whereby some arguments in
write_city_configurationwere over the 300 character threshold. This issue occurred when passing dictionary arguments without unpacking them. This PR makes thewrite_city_configurationfunction sensitive to dictionary arguments and unpacks them, assigning sensible names. I have also added a case for this in the test function.