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

BUG: Return empty list for empty default tag for vector parameter #6

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

Conversation

fbudin69500
Copy link

When the default value for a parameter that is a vector is not set
or set to nothing, it should be configured as an empty list, not
an empty string. If ctk_cli returns an empty string, the parsing
of the default value will result in an error.

When the default value for a parameter that is a vector is not set
or set to nothing, it should be configured as an empty list, not
an empty string. If ctk_cli returns an empty string, the parsing
of the default value will result in an error.
@cdeepakroy
Copy link

@fbudin69500 Maybe generate a warning and then assign it to empty list?

@cdeepakroy
Copy link

I feel if their xml does not provide a default for an optional argument, they should at least see a warning. This would preserve backward compatibility and also softly prompt them to fix their xml specs.

I personally would like to through an error which would force them to fix their xml specs. Not many python CLIs out there yet.

@cdeepakroy
Copy link

CC: @jcfr

@fbudin69500
Copy link
Author

I wouldn't throw an error. I think it is not wrong to specify an empty default value for vectors as it may very well be what you want (if by default you want to not give any element). Do you see any use-case where an empty list would create a problem? I feel that both in C++ and in Python, it would be valid to have an empty default value for a vector element. For other types of arguments (integer, floats,bolean), I agree that a default value should be required (it might be the case, I have not double-checked).

@cdeepakroy
Copy link

I agree with what you say for -vector types. As you mention, it becomes a bit of an issue for non-vector types.

But with scalar types, i foresee usecases in which they would like to delegate the default value selection to the actual code rather than specifying it in the xml spec. Think of minIntensity for which you might want to use as default the minimum possible value of the pixel type of the input image. Likewise for maxIntensity.

So i guess maybe the all inclusive way is to allow them not to specify the default in the xml spec. If it is not specified, within ctk_cli we assume and assign its corresponding variable to None. The CLI code will then have to check for None and override if needed. The only downside the user of the CLI will not know the default value used unless he looks at the CLI code.

@fbudin69500
Copy link
Author

I think the most important is to have a consistant behavior with GenerateCLP. @jcfr , what is the behavior of GenerateCLP if no default value is specified?

@hmeine
Copy link
Contributor

hmeine commented May 10, 2016

This is a problem I had as well. IIRC, GenerateCLP gives less arguments to the CLI than my MeVisLab backend in some cases. Maybe this is such a case, i.e. when no default is configured? In MeVisLab, it might be difficult to detect whether the user has "entered something", i.e. when using a widget for coordinates. When no default is given, such a widget would be filled with zeroes, and I would call the CLI with the (0,0,0) vector, for instance.

@fbudin69500
Copy link
Author

Hans,

Does the patch I submitted break something in MeVisLab? Or would it help in
case no default is given?

Francois

On Tue, May 10, 2016 at 2:49 AM, Hans Meine [email protected]
wrote:

This is a problem I had as well. IIRC, GenerateCLP gives less arguments to
the CLI than my MeVisLab backend in some cases. Maybe this is such a case,
i.e. when no default is configured? In MeVisLab, it might be difficult to
detect whether the user has "entered something", i.e. when using a widget
for coordinates. When no default is given, such a widget would be filled
with zeroes, and I would call the CLI with the (0,0,0) vector, for instance.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6 (comment)

@hmeine
Copy link
Contributor

hmeine commented May 10, 2016

I don't use the CLP in MeVisLab. I created ctk-cli to contain the code for calling CLIs, not for implementing them. It's nice that it now turns out to be useful for both sides, but the latter I have not done yet with MeVisLab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants