-
Notifications
You must be signed in to change notification settings - Fork 14
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
Use multiple external repos for definitions and mappings #311
Use multiple external repos for definitions and mappings #311
Conversation
Looks like we're having some Windows issues... I hope I can resolve them quickly. |
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.
Looks good in principle, a few suggestions inline
return {v} | ||
case list(v): | ||
return set(v) | ||
case set(v): |
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.
Do we want to allow a set type?
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 want to cast to set ultimately. If the user already provides a set we just pass it through. This case is pretty rare though since we usually create our NomenClatureConfig
from a yaml file so the input value there will always be a string or a list of strings.
For clarity I should switch the order though.
Co-authored-by: Daniel Huppmann <dh@dergelbesalon.at>
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.
Looks good, thanks!
Closes #295, closes #313.
This PR allows for using multiple external repos combined in a single dimension.
The complete test I implemented for that pulls from common-definitions and legacy-definitions.
Since legacy-definitions currently contains duplicates from common-definitions initiating a CodeList will result in an error. The test tests for this error.
Once this PR is merged, I'll do the following:
All of that should be possible today.