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

Fix spurious validation error with scatter+conditionals #1640

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tetron
Copy link
Member

@tetron tetron commented Mar 7, 2022

Typing related to conditionals should be part of the computed output
signature of the step, not added in by the checker.

refs #1639

Typing related to conditionals should be part of the computed output
signature of the step, not added in by the checker.

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>
@tetron tetron changed the title Fix spurious validation error #1639 Fix spurious validation error with scatter+conditionals Mar 7, 2022
@codecov
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Merging #1640 (21da544) into main (0f70992) will decrease coverage by 0.02%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1640      +/-   ##
==========================================
- Coverage   66.82%   66.79%   -0.03%     
==========================================
  Files          93       93              
  Lines       16578    16578              
  Branches     4404     4404              
==========================================
- Hits        11078    11074       -4     
- Misses       4361     4364       +3     
- Partials     1139     1140       +1     
Impacted Files Coverage Δ
cwltool/checker.py 85.02% <ø> (-1.37%) ⬇️
cwltool/workflow.py 85.32% <77.77%> (-0.33%) ⬇️
cwltool/cwltool/checker.py 86.78% <0.00%> (-0.45%) ⬇️
cwltool/cwltool/workflow.py 75.67% <0.00%> (+0.77%) ⬆️

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 0f70992...21da544. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Mar 7, 2022

This pull request introduces 1 alert when merging c4442a8 into 0f70992 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Mar 8, 2022

This pull request introduces 1 alert when merging 21da544 into 0f70992 - view on LGTM.com

new alerts:

  • 1 for Unused import

for oparam in outputparms:
if conditional and n == 0:
oparam["type"] = ["null"] + cast(
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test case that triggers this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was going to add a test, I just wanted to see what the other code checks had to say about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants