-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
add NamespacedTagSet api #22036
add NamespacedTagSet api #22036
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @benpankow and the rest of your teammates on |
e92fd80
to
5a2e9f7
Compare
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.
Nice.
There's a decent bit of duplication here with NamedspacedMetadataSet
. We shouldn't force it, but have you thought about whether there's a way to cut that down? E.g. a common superclass?
@@ -107,6 +107,7 @@ Pipfile.lock | |||
.mypy_cache/ | |||
|
|||
tags | |||
!python_modules/dagster/dagster/_core/definitions/tags |
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.
what's this?
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 gitignore all folders named tags
right now, I want to not gitignore this module
python_modules/dagster/dagster/_core/definitions/tags/tag_set.py
Outdated
Show resolved
Hide resolved
python_modules/dagster/dagster/_core/definitions/tags/tag_set.py
Outdated
Show resolved
Hide resolved
python_modules/dagster/dagster/_core/definitions/tags/tag_set.py
Outdated
Show resolved
Hide resolved
Args: | ||
tags (Mapping[str, str]): A dictionary of tags. | ||
""" | ||
kwargs = {} |
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.
Have you thought about whether there are ways to avoid code duplication between this and the same method in NamespacedMetadataSet
?
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.
Updated to use NamespacedKVSet
parent class
5a2e9f7
to
f076dd7
Compare
Ended up w/ a shared superclass, that cleans up both impls quite a bit I think |
7fe3de9
to
f551cf9
Compare
b69ffc9
to
29f89f5
Compare
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.
Nice. A couple small comments to look at before merging.
def extract( | ||
cls: Type[T_NamespacedMetadataSet], metadata: Mapping[str, Any] | ||
) -> T_NamespacedMetadataSet: | ||
def extract(cls: Type[T_NamespacedKVSet], metadata: Mapping[str, Any]) -> T_NamespacedKVSet: | ||
"""Extracts entries from the provided metadata dictionary into an instance of this class. |
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.
"""Extracts entries from the provided metadata dictionary into an instance of this class. | |
"""Extracts entries from the provided dictionary into an instance of this class. |
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.
Other sites in this docstring need to be updated.
python_modules/dagster/dagster/_core/definitions/tags/tag_set.py
Outdated
Show resolved
Hide resolved
bbd9c55
to
ff6489c
Compare
Merge activity
|
ff6489c
to
f101e67
Compare
## Summary Largely clones `NamespacedMetadataSet` into a new `NamespacedTagSet` ABC which can be used to define a set of tags which will be logically set together in code, and which have a namespace prefix. A bit simpler, in that all values must be strings. ## Test Plan New little unit test suite.
Summary
Largely clones
NamespacedMetadataSet
into a newNamespacedTagSet
ABC which can be used to define a set of tags which will be logically set together in code, and which have a namespace prefix.A bit simpler, in that all values must be strings.
Test Plan
New little unit test suite.