galaxy-iuc / standards

Documentation for standards and best practices from the Galaxy IUC
http://galaxy-iuc-standards.readthedocs.io/en/latest/
6 stars 16 forks source link

Use `<section>` for advanced options #42

Closed nsoranzo closed 6 years ago

nsoranzo commented 7 years ago

Fix https://github.com/galaxy-iuc/standards/issues/38 . Also clarify "Booleans" section.

gregvonkuster commented 7 years ago

I'm a bit confused about how the <section> tag should replace the use of the "Advanced options" <conditional> tag in many existing cases.

Many tools use the <conditional> tag to not only hide / display advanced options on the tool form, but also to not pass them on the command line if the conditional is "basic" (i.e., the Advanced options are hidden).

Does the <section> tag allow for this? From the examples I've seen, it seems to only hide / display the advanced options on the tool form, but always passes the default values of the advanced options on the command line even if the tag is not "expanded". This can break existing tools that were written to set missing parameter values on the command line to None in underlying scripts.

I very likely am just missing something here, but wanted to make sure.

nsoranzo commented 7 years ago

From the examples I've seen, it seems to only hide / display the advanced options on the tool form, but always passes the default values of the advanced options on the command line even if the tag is not "expanded".

That's correct. But I don't think many existing tools would exhibit the problem you are mentioning. Anyway:

gregvonkuster commented 7 years ago

I think there may be quite a few existing tools that use either the <conditional> tag or a <select> tag for hiding / displaying Advanced options, but perhaps many could be changed to accommodate a <section> tag.

But if new tools have to use the <section> tag for this feature, how will it work in the scenario where the underlying package produces differing results based on what it receives on the command line? I think many packages accommodate optional parameters on the command line, and behave differently if values for the optional parameters are received or not. Since the <section> tag does not allow for not passing the optional parameters, the underlying tool will always receive them with whatever default values are used by the tool developer. This could be misleading to the user because they are likely not aware that default values will always be passed to the underlying tool even when the Advanced Options are hidden.

nsoranzo commented 7 years ago

Shouldn't the tool developer use as default for a <param> the value indicated by the underlying tool as default? Passing --max default_for_max or not passing the option should be equivalent. If this is not the case, then you should probably keep using a conditional.

See also #38 for more reasons for this PR.

gregvonkuster commented 7 years ago

Perhaps in some cases (maybe most, but perhaps not), the underlying tool will specify a value as a default if an optional parameter is not passed. But in some cases (perhaps many), the underlying tool will not set a default value, but will instead perform an analysis that differs based on whether the optional parameter value was passed or not.

It looks like https://github.com/galaxy-iuc/standards/issues/38 is looking for best practices for a label for hiding / displaying advanced options, but defining the best practice as using the <section> tag for this takes it beyond labels because it changes the behavior of the tool.

I would argue that the <section> tag could be misleading as well, because if the section is not expanded, the user will likely assume that nothing the section contains will be passed on the command line.

The <section> tag can certainly be defined as a best practice here when possible - I can certainly see scenarios where it is best. But should the use of the <conditional> also be included here as a best practice for those cases where the <section> tag does not work, with clearly specified best practice labels?

hexylena commented 7 years ago

IUC Meeting: This should be changed to a softer recommendation.

nsoranzo commented 6 years ago

@gregvonkuster @erasche I have revised the <section> recommendation as decided during the last IUC meeting at GCC2017.

hexylena commented 6 years ago

:+1: