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
Merged

Add cwl checker #1361

merged 1 commit into from Dec 5, 2017

Conversation

tom-tan
Copy link
Contributor

@tom-tan tom-tan 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
Copy link

CLAassistant 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"
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

tom-tan 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
Copy link
Contributor Author

tom-tan commented Dec 4, 2017

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

Copy link
Member

@fmdkdd fmdkdd left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

This needs a --user flag.

@fmdkdd
Copy link
Member

fmdkdd 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
Copy link
Contributor Author

tom-tan commented Dec 5, 2017

Done!

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

fmdkdd commented Dec 5, 2017

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

@tom-tan
Copy link
Contributor Author

tom-tan commented Dec 5, 2017

Thank you for merging!

@tom-tan tom-tan deleted the add-cwl branch December 5, 2017 09:20
@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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants