Skip to content
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/alt circle avoidance #1553

Closed
wants to merge 2 commits into from
Closed

Conversation

MFizz
Copy link
Contributor

@MFizz MFizz commented Jul 29, 2016

Short Description:
alternative fix to PR #1547 while keeping the possibility to have nested keywords

spin_forts was assigned twice in config

Fixes:
#1544

circle avoidance now works as intended

@MFizz MFizz mentioned this pull request Jul 29, 2016
@douglascamata
Copy link
Member

There is more elegant solution to this by modifying add_config to make another parameter, maybe named embedded_in, to store this kind of information.

I don't think replacing a simple dict.get with a recursive search function is something good in terms of code complexity. And it also hides from add_config logic that the key is inside another.

Your code is prone to bugs like, another key in the dict root has a subkey with the same name. You will never search in the correct place because you take the first subkey you find that matches the name you want.

@MFizz
Copy link
Contributor Author

MFizz commented Jul 29, 2016

There is more elegant solution to this by modifying add_config to make another parameter, maybe named embedded_in, to store this kind of information.

Agreed. My idea originally was to keep the key unique regardless of depth, since cli/ flag functionality would dictate it anyway and to use upper levels only for grouping logically. But I can see how this feels dirty/ hackish. My problem now is just how to deal with the json - cli duality and that long_flag is used as attribute name and clie flag.
If you have an additional parameter embedded, how do you convert that to cli? At this point wouldn't it make more sense to just stay with a plain config file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants