ENH Add BaseTag handling in _tag_matches #59
Conversation
Codecov Report
@@ Coverage Diff @@
## main #59 +/- ##
==========================================
- Coverage 88.93% 88.78% -0.15%
==========================================
Files 14 14
Lines 1518 1525 +7
==========================================
+ Hits 1350 1354 +4
- Misses 168 171 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -560,6 +561,8 @@ def _tag_id_string(tag_id): | |||
|
|
|||
| @staticmethod | |||
| def _tag_matches(tag_value, operator, values): | |||
| if type(tag_value) == BaseTag: | |||
| values = IODValidator._value_basetag_conversion(values) | |||
There was a problem hiding this comment.
While this will fix the issue, I would prefer to fix the parser instead, so that the created json array can be correctly parsed. I checked that this problem appears only 4 times in the json file, and rather than add this check each time I would like to make it unnessary.
If you want to have a go at this, you are welcome of course, otherwise I will have a go (hopefuly soon).
In case you want to do this, I would recommend to write a test for the problem first (check the tests in test_part3_reader), and then write a fix against the test.
There was a problem hiding this comment.
Makes sense to me! I'm not sure exactly when I'll have time to work on this again, but I'll give it a go unless you get to it before me!
There was a problem hiding this comment.
Ok, thanks! I'm also not sure when I will get the time - best case would be this weekend... I will let you know!
There was a problem hiding this comment.
I had a preliminary look at this, and this may be a little bit trickier than I thought - I also encountered a couple of other conditions that are wrong due to the same parsing issue. So I think I'll better tackle this myself, but thanks for your help! You can of course go with your fix as long as I haven't done it.
There was a problem hiding this comment.
Sounds good! Thanks for all your work on dicom-validator, it's very appreciated!
|
Superseded by #61 as discussed - closing. |
See Issue 58