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

Lint extra #350

Merged
merged 9 commits into from
Oct 27, 2015
Merged

Lint extra #350

merged 9 commits into from
Oct 27, 2015

Conversation

hexylena
Copy link
Member

Whoops. Forgot to open the PR. Should close out some old issues.

little to no motivation to add tests ... so ... yeah.

conditional_selects = tool_xml.findall("./inputs//conditional")
for conditional in conditional_selects:
select = conditional.find('./param[@type="select"]')
# Should conditionals ever not have a select?
Copy link
Member

Choose a reason for hiding this comment

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

It can have a boolean. According to https://github.com/galaxy-iuc/standards/blob/master/docs/best_practices/tool_xml.rst#booleans we do not recommend to use booleans for a conditional but it is possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

interesting. Thanks for that, I'll update it accordingly

@hexylena
Copy link
Member Author

Comments addressed.

'command',
'inputs',
'request_param_translation',
'uihints',
Copy link
Member

Choose a reason for hiding this comment

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

Are uihints even respected for data source tools? Hmm... we can revisit that later though.

Can you add macros and configfiles back in the same order as normal tools?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an awful good question regarding uihints.

Yes, sorry, I removed everything I didn't see during very brief greps through the tools directory.

@jmchilton
Copy link
Member

Good changes, I'll just create an issue to add tests after it is merged.

@hexylena
Copy link
Member Author

@jmchilton okay, sounds good. Thanks for being lenient :)

@jmchilton
Copy link
Member

I'm going to merge this then - we will figure out Travis again someday I guess.

jmchilton added a commit that referenced this pull request Oct 27, 2015
@jmchilton jmchilton merged commit 088572a into master Oct 27, 2015
@jmchilton jmchilton deleted the lint-extra branch October 27, 2015 16:57
This was referenced Oct 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants