Skip to content

Conversation

@mr-c
Copy link
Member

@mr-c mr-c commented May 6, 2022

(Partial) implementation of common-workflow-language/common-workflow-language#764
Requires common-workflow-language/schema_salad#535

  • more tests & examples
  • min/max number of items in arrays

See also common-workflow-language/schema_salad@0acde77

@mr-c mr-c changed the title draft schema for paramter restrictions extension draft schema for input paramater restrictions extension May 6, 2022
@mr-c mr-c force-pushed the input_restrictions_ext branch from 72e56ba to a3b0e77 Compare May 6, 2022 13:10
@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

Merging #1665 (2ba09a2) into main (84a297f) will decrease coverage by 11.74%.
The diff coverage is n/a.

❗ Current head 2ba09a2 differs from pull request most recent head abf86c7. Consider uploading reports for the commit abf86c7 to get more accurate results

@@ Coverage Diff @@ ## main #1665 +/- ## =========================================== - Coverage 66.86% 55.12% -11.75%  =========================================== Files 93 47 -46 Lines 16622 8376 -8246 Branches 4414 2185 -2229 =========================================== - Hits 11114 4617 -6497  + Misses 4366 3203 -1163  + Partials 1142 556 -586 
Impacted Files Coverage Δ
cwltool/cwltool/process.py 67.34% <0.00%> (-3.63%) ⬇️
cwltool/cwltool/job.py 61.85% <0.00%> (-0.80%) ⬇️
cwltool/cwltool/main.py 32.08% <0.00%> (-0.09%) ⬇️
cwltool/main.py
cwltool/process.py
cwltool/command_line_tool.py
cwltool/pack.py
cwltool/resolver.py
cwltool/task_queue.py
cwltool/sandboxjs.py
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84a297f...abf86c7. Read the comment docs.

@tetron
Copy link
Member

tetron commented May 6, 2022

Is this related to the CTD mapping/translation effort?

@mr-c
Copy link
Member Author

mr-c commented May 6, 2022

Is this related to the CTD mapping/translation effort?

Yes, this is the beginning of an implementation of the input value restriction proposal inspired by CTD

Basically to get the checks working. I'd like to develop a more concise syntax either before the extension is released, or before the proposal to CWL v1.3+ is made

@tetron
Copy link
Member

tetron commented May 6, 2022

Is there documentation you can link to specifically for the CTD value restriction model?

The 2018 proposal seems a little over-complicated with different types of intervals.

I started working on a simpler proposal here but haven't followed up on it:

https://github.com/common-workflow-language/schema_salad/tree/constraints

@mr-c
Copy link
Member Author

mr-c commented May 6, 2022

Is there documentation you can link to specifically for the CTD value restriction model?

Nothing in their XSD, alas https://github.com/WorkflowConversion/CTDSchema/blob/5ab34f57cc18cf472aac109e9bdde0e40855dda2/Param_1_7_0.xsd#L39

Digging through the CTDConverter & CTDOpts code we learn that the restrictions field is either a comma-separated list of allowable values, or a simple numeric range separated by a colon

https://github.com/WorkflowConversion/CTDopts/blob/eec3d874bcf1ff66724cf3d7cda2dd6a70292e97/CTDopts/CTDopts.py#L173

It is the CTD people who asked for more control over intervals in the 2018 proposal; is each end of the interval exclusive or inclusive?

@lgtm-com
Copy link

lgtm-com bot commented May 11, 2022

This pull request introduces 1 alert when merging c6c9fe5 into ef7cb30 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
@mr-c mr-c force-pushed the input_restrictions_ext branch from af87ecc to 3ca50e0 Compare May 18, 2022 17:16
@lgtm-com
Copy link

lgtm-com bot commented May 18, 2022

This pull request introduces 1 alert when merging 2ba09a2 into ef7cb30 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Looking good!

@mr-c mr-c force-pushed the input_restrictions_ext branch from 2ba09a2 to abf86c7 Compare May 25, 2022 16:54
@mr-c mr-c marked this pull request as draft May 25, 2022 17:06
@lgtm-com
Copy link

lgtm-com bot commented May 25, 2022

This pull request introduces 1 alert when merging abf86c7 into 84a297f - view on LGTM.com

new alerts:

  • 1 for Unused local variable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants