-
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
Filter external repos #396
base: main
Are you sure you want to change the base?
Filter external repos #396
Conversation
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.
A few suggestions inline.
As with my other review, I’d strongly advise against using fnmatch.
- name: "Population*" | ||
tier: 1 | ||
exclude: | ||
- name: "Final Energy|*|*" |
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.
This is a misleading example - it seems to show only level-2 exclusion when in fact it excludes all variables at level 2 or below. Better to use the level-argument explicitly.
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.
Seems that this is subjective then. For me it was totally clear that this excludes anything level 2 and beyond.
I can see your point though about this being ambiguous
@@ -0,0 +1,17 @@ | |||
repositories: |
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.
Suggest to add more structure to the validation test data by using subfolders.
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.
Sure, good idea.
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.
#399 implements a cleanup of the test data folder, once that PR is merged, I'll rebase this one
@@ -126,12 +181,22 @@ def repos(self) -> dict[str, str]: | |||
} | |||
|
|||
|
|||
class MappingRepository(BaseModel): | |||
name: str |
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.
Das The mapping will also have to inherit the region-filters, right? Otherwise, a model could map to a region that is not included in the DataStructureDefinition.
@@ -5,5 +5,5 @@ repositories: | |||
url: https://github.com/IAMconsortium/legacy-definitions.git/ | |||
mappings: | |||
repositories: | |||
- common-definitions | |||
- legacy-definitions | |||
- name: common-definitions |
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.
Is the name now required? Could cause a lot of headache of that is now mandatory…
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.
As I wrote in the description of the PR it would be required as it stands now.
I can change the pydantic parsing so that it can still be used without the name attribute.
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.
Yes, better to add a field-validator that translates a string to name: {value}
to avoid breaking current workflows
Closes #326.
This PR adds the feature to filter external repositories using
include
andexclude
filters.Any number of filters combining any attributes can be defined, example:
For the variable section
in the example above we are including:
From this list we are then excluding all variables that match "Final Energy||".
This means that the final resulting list will contain no Final Energy variables with
three or more levels.
For the region section
we are taking only R5 and R10 regions.
Changes
One of the changes that I have made is that all repositories in the definition section need to have the
name
key-word.This would be a breaking change so all workflow repos that use external repositories would need to be updated.
I'd be happy to streamline it again so that just the name is allowed but for code simplicity I opted against that for now.