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 loadContents for File[] #2036

Merged

Conversation

fmigneault
Copy link
Contributor

When a type: File[] is provided with loadContents: true, the contents property of the respective files are NOT loaded. This does not respect the specification https://www.commonwl.org/v1.2/CommandLineTool.html#CommandInputParameter that indicates they should be loaded.

@mr-c
I would really like to have this integrated as soon as possible, since this is blocking me for many integrations with geospatial community tools. Feel free to adjust, or let me know what I just adjust, to make this integrated quickly (I do not remember the procedure to integrate the test to the test pipeline).

The expected output of the provided test is {"data": [1,2]}.
Without the changes, it results in {"data":[null,null]} (since contents is missing).

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.77%. Comparing base (4046c49) to head (a492285).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2036      +/-   ##
==========================================
- Coverage   83.80%   83.77%   -0.03%     
==========================================
  Files          46       46              
  Lines        8267     8267              
  Branches     2201     2201              
==========================================
- Hits         6928     6926       -2     
- Misses        859      861       +2     
  Partials      480      480              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fmigneault added a commit to crim-ca/weaver that referenced this pull request Aug 26, 2024
@mr-c
Copy link
Member

mr-c commented Aug 27, 2024

Thank you @fmigneault ! Did you mean to add test code that uses the new test data in this PR?

@fmigneault
Copy link
Contributor Author

Yes. The test data submitted is used by the test CWL.

@fmigneault
Copy link
Contributor Author

@mr-c
I think I might have misunderstood your question originally.
Indeed, there was no Python script added to run the test.
Now added.

@fmigneault
Copy link
Contributor Author

@mr-c All tests resolved and working.

@fmigneault
Copy link
Contributor Author

@mr-c
When is the next planned release with this PR integrated?

@mr-c
Copy link
Member

mr-c commented Aug 29, 2024

@mr-c
When is the next planned release with this PR integrated?

We don't have scheduled releases. Can you install from your fork?

@fmigneault
Copy link
Contributor Author

Yes, I can work with the fork in the meantime.
No rush for the release. Just trying to estimate when a tag would become available to pin the official repository.

@mr-c mr-c enabled auto-merge (squash) September 9, 2024 13:52
@mr-c mr-c disabled auto-merge September 9, 2024 16:35
@mr-c mr-c merged commit 4bb5329 into common-workflow-language:main Sep 9, 2024
49 of 50 checks passed
@mr-c
Copy link
Member

mr-c commented Sep 9, 2024

@fmigneault https://github.com/common-workflow-language/cwltool/releases/tag/3.1.20240909164951 :-)

@fmigneault
Copy link
Contributor Author

@mr-c Thanks!

@fmigneault fmigneault deleted the fix-load-contents-array branch September 10, 2024 00:45
@fmigneault fmigneault restored the fix-load-contents-array branch September 10, 2024 00:46
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.

loadContents does not load contents for an array of files
2 participants