set default tag sorting order #733
Conversation
| @@ -12,8 +12,7 @@ | |||
| class XarrayDataArrayConverter(WeldxConverter): | |||
| """Serialization class for xarray.DataArray.""" | |||
|
|
|||
| name = "core/data_array" | |||
| version = "0.1.0" | |||
| tags = ["asdf://weldx.bam.de/weldx/tags/core/data_array-0.1.*"] | |||
There was a problem hiding this comment.
should these now be tuples as well (as the type hint in the base class suggests.). I added this in #727, since I thought it make more sense to indicate that this is immutable.
There was a problem hiding this comment.
true, probably tuple is fitting
I only used list here because this is how it is in the asdf codebase.
I think the tags attribute gets 'resolved' in asdf and ends up as a list anyway. I don't know if that breaks with tuples here.
Frankly I would just stick to what asdf is doing to avoid any troubles if we can
|
please feel free to make further changes and/or merge @marscher As long as the tests pass we should be good here 👍 |
Codecov Report
@@ Coverage Diff @@
## master #733 +/- ##
==========================================
- Coverage 95.88% 95.88% -0.01%
==========================================
Files 92 92
Lines 6496 6489 -7
==========================================
- Hits 6229 6222 -7
Misses 267 267
Continue to review full report at Codecov.
|
|
|
||
| def to_yaml_tree(self, obj, tag: str, ctx: SerializationContext): | ||
| raise NotImplementedError | ||
|
|
||
| def from_yaml_tree(self, node: dict, tag: str, ctx: SerializationContext): | ||
| raise NotImplementedError | ||
|
|
||
| def select_tag(self, obj, tags, ctx): | ||
| return sorted(tags)[-1] |
There was a problem hiding this comment.
Can we be sure, that the tags/objs involved are part of weldx? Should we make this assertion here, or in converters deriving from this, treating a special case?
There was a problem hiding this comment.
we cannot really be sure of this here in cases where we also support 'reading' other tags into weldx properties (e.g. asdf/astropy quantities)
Right now I have implemented this manually in places where we support other tags
weldx/weldx/tags/units/pint_quantity.py
Lines 40 to 42 in 96d9bbc
This code is probably a more reasonable default so there is one less thing to worry about in the Converter implementation (and if we ever want to write out a non weldx tag we can implement this in the converter)
To be extra save we could also guard the list sorting like so
def select_tag(self, obj, tags, ctx):
weldx_tags = [tag for tag in tags if tag.startswith(WELDX_TAG_URI_BASE)]
if weldx_tags:
return sorted(weldx_tags)[-1]
return sorted(tags)[-1] 
Changes
select_tagforWeldxConverterChecks