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

fix: force load non-standard DICOM files #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ziqiangxu
Copy link

No description provided.

Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution 👍

My main comment here is that this probably should be an optional parameter to the constructor with the default being False so we don't change the behavior of the API. Perhaps also there should be a setter so people can turn it on or off for a given client instance.

Also, just as a matter of convention we like to have more verbose commit messages that describe the motivation and design considerations for the suggested change. Doing this in an issue can help but for something simple like this providing more a more detailed commit message will let us discuss any issues here in the PR thread.

@CPBridge @hackermd or others may also want to comment.

@CPBridge
Copy link
Collaborator

Agreed, I would certainly not want force loading of non-standard files to be default behaviour here. If we want to implement this, we should add a force parameter, and pass it down from the various top-level public methods (retrieve_study(), retrieve_series(), retrieve_instance() etc). @ziqiangxu would you be interested in making those changes

@CPBridge CPBridge added the enhancement New feature or request label Oct 19, 2024
Copy link
Collaborator

@CPBridge CPBridge left a comment

Choose a reason for hiding this comment

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

This would need to be an optional parameter, with default False, and be passed down from the various public methods that call _http_get_multipart_application_dicom

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants