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

dcm4che/dcm4chee-arc-light#799 : dcm4che-tools-findscu: query params mentioned in dicom binary file or xml file is unused if specified after -m, -i or -r flags #118

Closed
wants to merge 1 commit into from

Conversation

vrindanayak
Copy link
Member

…mentioned in dicom binary file or xml file is unused if specified after -m, -i or -r flags
@vrindanayak vrindanayak added this to the 3.3.9 milestone Jun 20, 2017
@vrindanayak vrindanayak self-assigned this Jun 20, 2017
@vrindanayak vrindanayak requested a review from hczedik June 20, 2017 12:11
if (optVals != null)
for (String key : optVals) {
File f = new File(key);
if (f.exists())
Copy link
Member

@hczedik hczedik Jun 20, 2017

Choose a reason for hiding this comment

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

Hi Vrinda,
this logic seems too complex. Why do you have to check if the argument is a file?
If option -r -m or -i is used, then the arguments following those options won't be a file, so e.g.:

findscu -i PatientName -i ModalitiesInStudy dicomfile.dcm

PatientName and ModalitiesInStudy are obviously not files, because they directly follow -i. On the other hand dicomfile.dcm is a file.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think I understand the problem now.
Maybe we should create these options with

.hasArg()

instead of

.hasArgs()

to avoid this ambigouity?

Just imagine the confusion should someone by conincidence have a file called e.g. "PatientName" in his working directory. Your current implementation is unpredictable.

Copy link
Member

Choose a reason for hiding this comment

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

.hasArg() would avoid the ambiguity but break the current behavior to do not need to repeat -r -m or -i for specifying multiple arguments for the option. I would vote for keeping the current behavior, and investigate the effort to support the common CLI convention using -- to separate option arguments from program parameters

Copy link
Member

Choose a reason for hiding this comment

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

Solved by #126

@gunterze gunterze removed this from the 3.3.9 milestone Jun 30, 2017
@gunterze gunterze closed this Jun 30, 2017
@gunterze gunterze deleted the dcm4chee-arc-light#799 branch June 30, 2017 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants