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

Deprecate `size` attribute of `<param/>` and remove it from tools #6684

Merged
merged 2 commits into from Dec 12, 2018

Conversation

Projects
None yet
5 participants
@nsoranzo
Copy link
Member

commented Sep 10, 2018

Also:

  • Remove an unused and broken tool tools/data_source/bed_convert.xml
  • dos2unix test/functional/tools/for_workflows/head.xml
  • Single-quote text and data params in <command/>
  • Remove deprecated interpreter attribute of <command />

nsoranzo added some commits Sep 6, 2018

Deprecate `size` attribute of `<param/>` and remove it from tools
Also:
- dos2unix test/functional/tools/for_workflows/head.xml
- Single-quote text and data params in `<command/>`
- Remove deprecated `interpreter` attribute of `<command />`
@jmchilton

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

I'm -0 on this. Clearly the docs should be updated to reflect this is unused. But in the abstract it seems like valid display metadata the tool author might want to capture - especially in the case of text area inputs. Galaxy not actually using it currently is pretty irrelevant in my mind - Galaxy tools shouldn't be artifacts designed to only be used by one specific runtime and shouldn't only capture metadata that runtime can currently consume intelligently. I understand other people feel differently about these points and I'm fine with this being merged.

@nsoranzo

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2018

Galaxy tools shouldn't be artifacts designed to only be used by one specific runtime and shouldn't only capture metadata that runtime can currently consume intelligently.

@jmchilton I think a reason to drop size is that it is actually a presentational detail which doesn't fit in the "abstract" tool description you support. In fact, there's no equivalent of size in the CWL command line tool description standard, AFAIK.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

Galaxy tools have many presentational aspects, this isn't a problem in my opinion and I think it is likely a strength of them versus other approaches. The counter argument against CWL made to me frequently is that it doesn't capture any presentation details - I am told it is a deficit of that specification and I find the argument not unpersuasive. Admittedly you've never made that argument to me, but others certainly have. Regardless, I'm not trying to make Galaxy tools into CWL tools - I'm trying to make them suitably abstract and detached from the specific implementation that is Galaxy - form input type, labels, and help seem appropriate and at roughly the same level at preferred text box size and shape.

@nsoranzo

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2018

form input type, labels, and help seem appropriate and at roughly the same level at preferred text box size and shape.

I generally support making Galaxy tools as abstract descriptions as possible, but I don't agree on the above sentence: text box size and shape are at the same level of text colour and font, IMO. In fact, input type and label are in the CWL tool description.

@guerler

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

In my view the size attribute is/was basically the size property of the HTML input dom field. It is a somewhat arbitrary UI suggestion and I'd be fine with removing it. It is true that tool configs contain representational properties for inputs like e.g. the display property of select fields or the hidden option. But these properties are more abstract and do not go into the detail of specifying a particular numeric value like a certain length, width or color.

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

Since there seems to be discussion to be had here, and it's not an urgent change, I'm going to re-milestone this for 19.01 so we can freeze.

@dannon dannon modified the milestones: 18.09, 19.01 Sep 10, 2018

@jmchilton jmchilton merged commit bf550da into galaxyproject:dev Dec 12, 2018

6 checks passed

api test Build finished. 420 tests run, 1 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 187 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 118 tests run, 5 skipped, 0 failed.
Details
selenium test Build finished. 144 tests run, 2 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@jmchilton

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Merged with serious reservations. Thanks though for the contribution, it is appreciated even if we don't agree.

@nsoranzo nsoranzo deleted the nsoranzo:deprecate_param_size branch Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.