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

Allow extending profiles and merging their contents. #592

Merged
merged 4 commits into from
Feb 28, 2020

Conversation

janstrohbeck
Copy link
Contributor

@janstrohbeck janstrohbeck commented Jan 13, 2020

This allows profiles to contain the following:

extends: <profile name>

The Context class then loads that base profile and merges the contents of the profiles (last value for a given key wins). When saving the context, the changes are saved in the correct files by keeping a lookup table which stores the file that each parameter was loaded from.

The use case I have for this is that we have many users, each with a separate profile. Most users just want to set a custom whitelist or blacklist and use default values from another profile for the cmake_args etc. With this PR, a lot of copied and pasted YAML code can be removed.

I do not know if this is something that is wanted in this project. If yes, I can add unit tests for the added functionality or refactor the changes if there is a nicer way to implement it (the key_origins feels a bit hacky).

Edit: extends might not be a good name as it is similar to extend_path. Also, I couldn't get the unit tests to work on my machine, so the CI build might fail.

@mikepurvis
Copy link
Member

This looks like a reasonable and useful contribution. I don't think I have much quibble with the name— extends, parent, base, they're all basically the same thing. We do use "extend" to refer especially to the business of extending a workspace, but that's pretty separate from config profiles.

I'm happy to see tests come in a follow-on. Is there anything you want to add to this before it goes in?

@janstrohbeck
Copy link
Contributor Author

We've used it at my organization for a couple of weeks and have not had any issues with it yet. It works even with 4 levels of extends :-)

So I think it can be merged as is. I probably won't have time to add tests in the next few weeks.

@mikepurvis
Copy link
Member

Awesome, thanks for the contribution.

@mikepurvis mikepurvis merged commit 856bf18 into catkin:master Feb 28, 2020
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