BAMWelDX / weldx

The welding data exchange format
https://www.bam.de/weldx
BSD 3-Clause "New" or "Revised" License
20 stars 10 forks source link

Fix wxextension matching #826

Closed marscher closed 1 year ago

marscher commented 1 year ago

Changes

asdf-2.14.0+ returns two Extension instances matching the pattern in get_weldx_extension(). So we allow this now and add a version mismatch check. Another minor change is to add caching to the function, as it is being called for some types, each time one of these types is being serialized. The cache hits, if the extension tuple matches (based on id(), I'd guess).

Related Issues

Closes #825

Checks

github-actions[bot] commented 1 year ago

Test Results

2 184 tests  ±0   2 183 :heavy_check_mark: +118   3m 6s :stopwatch: +50s        1 suites ±0          1 :zzz: ±    0         1 files   ±0          0 :x:  - 113 

Results for commit 6d5166ad. ± Comparison against base commit a617b06e.

:recycle: This comment has been updated with latest results.

codecov[bot] commented 1 year ago

Codecov Report

Merging #826 (6d5166a) into master (a617b06) will decrease coverage by 0.03%. The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master     #826      +/-   ##
==========================================
- Coverage   97.25%   97.21%   -0.04%     
==========================================
  Files          82       82              
  Lines        4913     4921       +8     
==========================================
+ Hits         4778     4784       +6     
- Misses        135      137       +2     
Impacted Files Coverage Δ
weldx/asdf/util.py 90.79% <72.72%> (-0.40%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

CagtayFabry commented 1 year ago

I am not sure if this is a bug on the asdf side or if something is wrong with the way we implemented our extension

It would be good to test the caching with the quality standard implementation to see if that has any effect

marscher commented 1 year ago

Taking a look in the debugger, other extensions, including asdf standard extensions have this splitted pattern of one extension carrying the tags and another containing the converters. So I doubt that you did something wrong adopting the new pattern (a manifest extension, right?)

marscher commented 1 year ago

If you are afraid, that caching breaks something, I can also just remove the implementation. Probably it doesn't make any difference. How would I test against the quality standard? I thought the default is activated on import time.

marscher commented 1 year ago

I expect that asdf changes the entrypoint listing implementation, so we do not have duplicates in this list.