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
bugfix/catch-invalid-XMLAnnotationID #455
bugfix/catch-invalid-XMLAnnotationID #455
Conversation
Codecov ReportBase: 94.00% // Head: 93.91% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #455 +/- ##
==========================================
- Coverage 94.00% 93.91% -0.10%
==========================================
Files 46 46
Lines 4021 4027 +6
==========================================
+ Hits 3780 3782 +2
- Misses 241 245 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
Looks good! Are there any tests we could add for 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.
Looks good to me!
@AetherUnbound I am okay without new test data being added. Is that a hard req from you or okay to merge? |
I could also mock some fake data without a file. I also fixed/test this in ome types tlambert03/ome-types#159 |
Not to confuse things, but should we just update ome-types min version and call it good? (No objection from me either way) |
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.
Nope! Not a dealbreaker, just thought I'd mention in case it was an easy thing to throw in 🙂
We will have to wait for @tlambert03's answer but I agree. If there is no difference between the two then why not? |
That works for me too! Let me also try to test in on windows, since the original poster reported there might still be an issue |
Does this seem good to merge? @tlambert03 |
actually, I think you can just close this ... it should have been fixed by tlambert03/ome-types#159 which has since been released in ome-types v0.3.3 |
I'll go ahead and do that for you :) if it comes up again feel free to ping me |
Description
fixes #454
This PR is sufficient to get the xml/h5 dataset linked in the original issue reading with the bioformats reader... (and lets me think a bit longer about how I want to handle invalid schemas over in ome-types)
Pull request recommendations:
Thanks for contributing!