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

Use multiple external repos for definitions and mappings #311

Merged

Conversation

phackstock
Copy link
Contributor

@phackstock phackstock commented Jan 22, 2024

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:

  1. Address Raise all duplicate-errors during initialization #104 so that all the duplicate codes are raised in a single error
  2. Fix legacy-definitions so that they are compatible with common-definitions
  3. At this point the multiple repo test here will fail since it will no longer produce an error so I'll fix the test.

All of that should be possible today.

@phackstock
Copy link
Contributor Author

Looks like we're having some Windows issues... I hope I can resolve them quickly.

Copy link
Member

@danielhuppmann danielhuppmann left a 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):
Copy link
Member

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?

Copy link
Contributor Author

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.

@phackstock phackstock added the enhancement New feature or request label Jan 22, 2024
Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@phackstock phackstock merged commit 87311de into IAMconsortium:main Jan 23, 2024
16 checks passed
@phackstock phackstock deleted the feature/multiple-external-repos branch January 23, 2024 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing repositories after tests fails on Windows Mix multiple external repos in a single dimension
2 participants