-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
[config] Merging lists #153
[config] Merging lists #153
Comments
Let's list the options we have so we can get closer to a solution.
@nemesisdesign What do you think? |
@okraits option 3. sounds like a good idea, an "advanced option" which allows to decide if the user wants to add to any previous template or override, to be set on a per template basis and it would act only on previous templates, right? |
Yes, each template would basically take the result of the previous templates as input and then override that or add to it, based on the selected advanced option. |
@nemesifier So if you're ok with option 3, I will start to work on it, ok? |
@okraits how do you intend to implement this? Are we going to add an initialization argument to the library? You mention adding an "option" to the list, can you expand on this? |
To be honest, I have to look into the code first to see what's possible. I'll get back to you with a proposal, ok? |
@nemesifier I thought about the whole topic and possible solutions. Originally I thought about adding a "meta setting" to every list in the json configuration dictionary (which would mean that we would have to store a list in json in a surrounding dictionary to be able to have a "meta setting") to define wether the corresponding list should be merged with or override an existing list. So I guess the more simple solution is to add a field to the AbstractTemplate model to define the behaviour of lists for each template. I can think of the following behaviours:
Am I correct that we basically need to modify the following functions to implement the behaviours mentioned above:
What do you think? |
@nemesifier Any opinion on my suggestion? |
@nemesifier I didn't receive a response from you, so I implemented this. Please take a look at it and feel free to ask questions or request changes. P.S. I will first wait for your feedback and fix the tests afterwards. |
The way configuration is merged with templates now has a quirk:
list elements from different templates are merged with the config.
This is needed for list of objects (eg: interfaces, routes), but it's weird and has caused misunderstanding among users when they try to override list options.
I still haven't come up with an optimal solution.
The text was updated successfully, but these errors were encountered: