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 #77: Unify v1*_only parameter into target_version #79

Merged
merged 7 commits into from
Nov 14, 2021
Merged

Fix #77: Unify v1*_only parameter into target_version #79

merged 7 commits into from
Nov 14, 2021

Conversation

tom-tan
Copy link
Member

@tom-tan tom-tan commented Nov 11, 2021

It introduces target_version parameter to replace v1*_only parameters.
It also make imports parameter as a default parameter to make target_version optional.

Note:

  • Current implementation prints an error message and returns nothing if a given CWL doc has unsupported cwlVersion. Can we leave it as is or fix it to raise an exception, for example?

@codecov
Copy link

codecov bot commented Nov 11, 2021

Codecov Report

Merging #79 (a7e29d9) into main (4fa21a6) will increase coverage by 12.36%.
The diff coverage is 67.39%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main      #79       +/-   ##
===========================================
+ Coverage   50.43%   62.80%   +12.36%     
===========================================
  Files           3        3               
  Lines         462      500       +38     
===========================================
+ Hits          233      314       +81     
+ Misses        229      186       -43     
Impacted Files Coverage Δ
cwlupgrader/main.py 60.25% <54.54%> (+12.06%) ⬆️
tests/test_complete.py 100.00% <100.00%> (ø)

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 4fa21a6...a7e29d9. Read the comment docs.

@mr-c
Copy link
Member

mr-c commented Nov 11, 2021

The imports set needs to be external of the function, as it is used to prevent duplicate upgrading of the same external file, when upgrading more than one primary document.

@tom-tan
Copy link
Member Author

tom-tan commented Nov 12, 2021

Thank you for your advice.
Now run calls upgrade_document with a set.
It prevents duplicate upgrading while keeping the API of upgrade_document simple.

@mr-c
Copy link
Member

mr-c commented Nov 12, 2021

Thank you for your advice. Now run calls upgrade_document with a set. It prevents duplicate upgrading while keeping the API of upgrade_document simple.

Thanks!

I'm seeing some code coverage issues; 22% is rather low. You can run make diff-cover locally to get a line-by-line report, or wait for the CI to finish and check https://github.com/common-workflow-language/cwl-upgrader/pull/79/files for annotations about lack of test coverage.

image

@tom-tan
Copy link
Member Author

tom-tan commented Nov 14, 2021

Added!

Note that the current implementation of upgrade_document only dumps the upgrade documents for steps fields and does not dump the document for a given CWL that is passed to the parameter (it is done by cwlupgrader.main.run).
Fixing this issue is out of scope of this request (will be fixed when #78 is solved).

@mr-c mr-c merged commit a5d65f2 into common-workflow-language:main Nov 14, 2021
@tom-tan tom-tan deleted the unify-target-version branch November 14, 2021 12:58
@tom-tan
Copy link
Member Author

tom-tan commented Nov 14, 2021

Thanks!

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