-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
Support YAML 1.2 better #458
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! +1
type: | ||
- type: array | ||
items: [string, File] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. This test appears to contain both options for declaring the values, with and without the ?
char 👍
assertEquals(wkflow.getInputs().get("ncrna_tab_file").getType(), "File?"); | ||
assertEquals(wkflow.getInputs().get("reverse_reads").getType(), "File?"); | ||
assertEquals(wkflow.getInputs().get("ssu_tax").getType(), "string, File"); | ||
assertEquals(wkflow.getInputs().get("rfam_models").getType(), "{type=array, items=[string, File]}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think other parts of the file use spaces instead of tabs? (didn't check the whole file, just the parts appearing here in GH UI, above and below your change).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I read this test before reading the CWLService change. That file appears to be using tabs. No need to change anything, the project appears to have a mix of both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the source format option in Eclipse. If you recommend a different formatter then I'll apply that to the entire codebase in a separate PR 👍
Description
Switch from snakeyaml to snakeyaml-engine
Motivation and Context
Fix underlying issue in #457 (but does not fix error reporting)
How Has This Been Tested?
The added test fails without the added code.
Screenshots (if appropriate):
Types of changes
Checklist: