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

Add cwl checker #1361

Merged
merged 1 commit into from Dec 5, 2017

Conversation

@tom-tan
Copy link
Contributor

commented Nov 20, 2017

This request adds a syntax checker for Common Workflow Language (CWL) that describes analysis workflows and tools.
Its syntax is defined as Semantic Annotations for Linked Avro Data (SALAD).
I added a syntax checker for CWL using schema-salad-tool (link to pip repository), which is a validator of SALAD format.

To test the CWL checker, I copied the schema files for CWL from the repository of schema salad, which are licensed under Apache-2.
I guess it is harmless to include these files to this repository for this purpose because Apache-2 and GPL-3 are compatible.

@CLAassistant

This comment has been minimized.

Copy link

commented Nov 20, 2017

CLA assistant check
All committers have signed the CLA.

(let ((flycheck-cwl-schema-path "schema/CommonWorkflowLanguage.yml"))
(flycheck-ert-should-syntax-check
"language/cwl/cwl.cwl" 'cwl-mode
'(6 5 error "the `inputBinding` field is not valid because value is a str, expected null or CommandLineBinding"

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Nov 20, 2017

Member

Seeing as there is only one expected error, do we really need to include all these files for testing? In other words, can you build a minimal test that triggers the same error?

@tom-tan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2017

Unfortunately, we can not reduce files in schema/ because they are referred by schema/CommonWorkflowLanguage.yml.

IMO it is difficult to remove other schema files.
If we modify the schema to remove the dependency, the test checks the message for modified CWL and it is not what we want to test.

@fmdkdd fmdkdd added this to Backlog in Maintenance Nov 22, 2017

@tom-tan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2017

ping.
Is it ready to merge or should we need a workaround not to use the schemas?

@fmdkdd
Copy link
Member

left a comment

LGTM; just a minor change to the Vagrantfile

Vagrantfile Outdated
@@ -171,6 +172,9 @@ Vagrant.configure("2") do |config|
typescript \
js-yaml
echo "Installing packages with pip..."
pip install schema-salad

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Dec 4, 2017

Member

This needs a --user flag.

@fmdkdd

This comment has been minimized.

Copy link
Member

commented Dec 4, 2017

@tom-tan I just needed to find some time to run the test locally. Thank you for adding one, by the way. This is a good PR all around.

Could you also add a line to CHANGES.rst under New syntax checkers? And squash your commits when you are done, then we can merge.

@fmdkdd fmdkdd moved this from Backlog to IP in Maintenance Dec 4, 2017

@tom-tan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

Done!

@fmdkdd

fmdkdd approved these changes Dec 5, 2017

@fmdkdd fmdkdd merged commit 697b22d into flycheck:master Dec 5, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@fmdkdd

This comment has been minimized.

Copy link
Member

commented Dec 5, 2017

Great! Nice work 👏 , and sorry for the delay.

@tom-tan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

Thank you for merging!

@tom-tan tom-tan deleted the tom-tan:add-cwl branch Dec 5, 2017

@fmdkdd fmdkdd removed this from IP in Maintenance Dec 6, 2017

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