CWL Parsing #22
CWL Parsing #22
Conversation
…wlgen into parsing
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
==========================================
+ Coverage 76.75% 83.13% +6.38%
==========================================
Files 9 10 +1
Lines 684 593 -91
==========================================
- Hits 525 493 -32
+ Misses 159 100 -59
Continue to review full report at Codecov.
|
| :type glob: STRING | ||
| :param load_contents: For each file matched, read up to the 1st 64 KiB of text and | ||
| place it in the contents field | ||
| :type load_contents: BOOLEAN |
There was a problem hiding this comment.
I notice this isn't an actual Python type (should be bool). Is this intentional? It doesn't seem ideal
There was a problem hiding this comment.
It's a legacy thing that I haven't changed. Might be worth changing in #24
There was a problem hiding this comment.
If/once you deprecate Python 2, you can make these all PEP 484 type annotations and then most tooling should work better
| @@ -15,13 +16,109 @@ | |||
| # Class(es) ------------------------------ | |||
|
|
|||
|
|
|||
| class CommandOutputBinding(Serializable): | |||
There was a problem hiding this comment.
These classes should probably be dataclasses, no? That would save you from having to define the __init__ methods all the time. Admittedly they require Python 3.6 (via backport) or Python 3.7 (natively). Not sure what versions you're supporting.
There was a problem hiding this comment.
Woah that's sick! I didn't know Dataclasses were a thing! When this project drops EOL support for Python 2.7 I reckon this is definitely the way to go.
| @@ -0,0 +1,111 @@ | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
Did you think about doing an integration test that does CWL → Python → CWL and comparing the input and output CWL?
There was a problem hiding this comment.
I think it would be a good test to improve this translation mechanism. For this change I know it won't produce identical CWL because it's opinionated on the dict / array output of inputs / outputs / steps.
It would be good to functionally validate the CWL tbh.
(not just convert the array of strings to a string "['value']" haha
|
Thanks @TMiguelT, all tests pass so I'm going to merge this! |
Hey all,
This is the start of my parsing a CWL into the workflow classes, with the goal for it to be almost automatic (with a couple of annotated hints).
Most of the work is in
utils, based on my previousSerializablesubmission.https://github.com/illusional/python-cwlgen/blob/parsing/cwlgen/utils.py#L69-L177
I make two assumptions:
KeyExceptionif this is not the case.Essentially, it loops through the keys on the CWL Dictionary and sets that attribute (if it exists) on the required class. You can provide an ordered list of "potential" classes that it might be, and it will do its best to go through until it finds one.
You can see how this works on the Workflow (L41-44) class.
I've swapped out the converter and left the CommandLineTool tests (from @khillion) the same (except for secondaryFiles) and it seems to work okay.
@kellrott, do you mind having a look at my code, as both yourself at @khillion are the people who have worked on workflow parsing, and this should just extend it to Workflow (for now).