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
[MRG] Prevent crash due to invalid private creator #1639
[MRG] Prevent crash due to invalid private creator #1639
Conversation
- issue a warning but don't change the private creator logic - fixes pydicom#1638
Codecov Report
@@ Coverage Diff @@
## master #1639 +/- ##
==========================================
+ Coverage 97.57% 97.67% +0.10%
==========================================
Files 66 66
Lines 10666 10674 +8
==========================================
+ Hits 10407 10426 +19
+ Misses 259 248 -11
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
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 - just one small suggestion.
@@ -553,6 +553,10 @@ def get_private_entry( | |||
f"Private creator '{private_creator}' not in the private " | |||
"dictionary" | |||
) from exc | |||
except TypeError as exc: | |||
msg = f"'{private_creator}' is not a valid private creator" | |||
warnings.warn(msg) |
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.
I think it would be helpful to have the tag number here as well as its value.
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.
Sorry, for some reason I missed that comment - adapted now.
Describe the changes
I decided to make a minimum impact change that only prevents the crash. Any invalid private creator will still be handled as a private creator with respect to the private tags.
The crash itself only happens due to the lookup of the key in the private dictionary, which now behaves the same as a valid private creator not present in the dictionary.
Tasks