search

bids-standard / bids-validator

Validator for the Brain Imaging Data Structure
https://bids-standard.github.io/bids-validator/
MIT License
181 stars 109 forks source link

fix: Use @libs/xml over xml2js for tiff parsing #2106

Closed nellh closed 1 month ago

nellh commented 1 month ago

This should work in browser builds too.

Resolves #2105

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (d232223) to head (786cc93). Report is 8 commits behind head on master.

Files Patch % Lines
bids-validator/src/files/tiff.ts 88.88% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2106 +/- ## ========================================== + Coverage 85.69% 90.04% +4.35% ========================================== Files 91 42 -49 Lines 3782 3195 -587 Branches 1220 436 -784 ========================================== - Hits 3241 2877 -364 + Misses 455 309 -146 + Partials 86 9 -77 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nellh commented 1 month ago

Working on potential fixes for the Vite issue still.

rwblair commented 1 month ago

My current ranking of preferred solutions is:

  1. This #2106 - Better xml library, closest to original solution, most maintainable.
  2. Chris' #2107 - Works, reduces dependencies. Unknown how robust it is for real world data.
  3. Mine #2108 - Ugly as sin, most complex to maintain, but keeps og library in place.

@nellh You have any hope of there being a work around for the vite import:glob problem?

nellh commented 1 month ago

@nellh You have any hope of there being a work around for the vite import:glob problem?

Narrowed this down to a bug in Vite itself, with the vite:asset-import-meta-url plugin. Added a small build plugin to patch the impacted code. I will try to get it fixed upstream but this should work in the meantime.

nellh commented 1 month ago

Vite upstream for this is here. https://github.com/vitejs/vite/issues/10597