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

Avoid multi-megabyte error message #1683

Merged

Conversation

chris-remedy
Copy link
Contributor

@chris-remedy chris-remedy commented Aug 24, 2022

Describe the changes

If the incorrect data was large, this error message was very large. Ended up with a 300MB error log output because of this on a single real dicom.

Tasks

  • Unit tests added that reproduce the issue or prove feature is working
  • Fix or feature added
  • Code typed and mypy shows no errors
  • Documentation updated (if relevant)
    • No warnings during build
    • Preview link (CircleCI -> Artifacts -> doc/_build/html/index.html)
  • Unit tests passing and overall coverage the same or better

If the incorrect data was large, this error message was very large. Ended up with a 300MB error log output because of this on a single real dicom.
@pep8speaks
Copy link

pep8speaks commented Aug 24, 2022

Hello @chris-remedy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-08-25 15:06:04 UTC

@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #1683 (a765a41) into master (19b4739) will increase coverage by 0.10%.
The diff coverage is n/a.

❗ Current head a765a41 differs from pull request most recent head f593359. Consider uploading reports for the commit f593359 to get more accurate results

@@            Coverage Diff             @@
##           master    #1683      +/-   ##
==========================================
+ Coverage   97.58%   97.68%   +0.10%     
==========================================
  Files          66       66              
  Lines       10712    10712              
==========================================
+ Hits        10453    10464      +11     
+ Misses        259      248      -11     
Impacted Files Coverage Δ
pydicom/values.py 97.40% <ø> (ø)
pydicom/valuerep.py 99.55% <0.00%> (+0.14%) ⬆️
pydicom/filewriter.py 98.06% <0.00%> (+0.24%) ⬆️
pydicom/filereader.py 94.62% <0.00%> (+0.25%) ⬆️
pydicom/uid.py 100.00% <0.00%> (+1.09%) ⬆️
pydicom/filebase.py 99.19% <0.00%> (+1.61%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -388,7 +389,8 @@ def convert_numbers(
if length % bytes_per_value != 0:
raise BytesLengthException(
"Expected total bytes to be an even multiple of bytes per value. "
f"Instead received {byte_string!r} with length {length} and "
f"Instead received {shorten(repr(byte_string), width=10_000)} "
f"with length {length} and "
f"struct format '{struct_format}' which corresponds to bytes per "
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this, it's a good idea to clip this output somehow.

But when I try shorten on some binary values I always get "[...]", so it's not very helpful.
E.g.

b = b"\5" * 100
shorten(repr(b), 50)
'[...]'

Also, I think 10_000 is still too large to display and be helpful.

I would suggest if length is less than say, a few hundred, print the whole repr, otherwise the message can skip the repr(byte_string) and just print the length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, how's that?

Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

Could you please add an entry to the Release Notes?

@chris-remedy
Copy link
Contributor Author

@mrbean-bremen done.

Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

Thanks!

@darcymason darcymason merged commit e77e658 into pydicom:master Aug 25, 2022
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.

4 participants