Skip to content
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] Catch missed test warnings, clean up validation warnings with DSdecimal #2107

Merged
merged 2 commits into from
Jul 21, 2024

Conversation

scaramallion
Copy link
Member

@scaramallion scaramallion commented Jul 20, 2024

Describe the changes

  • Catches missed logged warnings in the tests
  • Update README to remove contrib-pydicom mention
  • Cleaner validation warning for DSdecimal (either too long warning, or invalid warning, not both)
  • Use original_string value with DSdecimal if auto_format is False... it was flagging DSdecimal("1.2345") as invalid.
  • Refactored the buffer test since it's taking a weirdly long time in the workflow.

The validation warnings with IS/DS are a bit messy. I think either the validation should be done externally by the validation checks in valuerep, or internally by the class __init__. Currently it's both, so you can get two warnings about the same invalid value.

Tasks

  • Unit tests added that reproduce the issue or prove feature is working
  • Fix or feature added
  • Code typed and mypy shows no errors
  • Unit tests passing and overall coverage the same or better

Copy link

codecov bot commented Jul 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.36%. Comparing base (d7f088e) to head (f2547e4).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2107      +/-   ##
==========================================
+ Coverage   98.32%   98.36%   +0.03%     
==========================================
  Files          76       76              
  Lines       13362    13362              
==========================================
+ Hits        13138    13143       +5     
+ Misses        224      219       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@scaramallion scaramallion changed the title [WIP] Catch missed test warnings, clean up validation warnings with DSdecimal [MRG] Catch missed test warnings, clean up validation warnings with DSdecimal Jul 21, 2024
@@ -1108,7 +1108,7 @@ def __str__(self) -> str:
return repr(self)[1:-1]

def __repr__(self) -> str:
if self.auto_format and hasattr(self, "original_string"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually don't understand in what situations autoformat is needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit of an odd thing, you'd only be able to use it if creating your own values, in which case you'd presumably just format them correctly beforehand?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I've thought, too... Maybe @darcymason knows more?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It came from #1264/#1334, just a shorthand way to allow users to fix DS problems automatically.

@scaramallion scaramallion merged commit a0ec842 into pydicom:main Jul 21, 2024
10 checks passed
@scaramallion scaramallion deleted the house-tests branch July 21, 2024 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants