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

scandeps should not apply mergedirs at every level #1615

Merged
merged 5 commits into from
Feb 10, 2022
Merged

Conversation

tetron
Copy link
Member

@tetron tetron commented Feb 9, 2022

When getting back a list of dependencies, and using "nestdirs", in the
specific case it there are references to both a directory and files
within that directory, it may report multiple dependencies for the
same directory. The mergedir method merges those references into one.

When getting back a list of dependencies, and using "nestdirs", in the
specific case it there are references to both a directory and files
within that directory, it may report multiple dependencies for the
same directory.  The mergedir method merges those references into one.
@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #1615 (f8a5d4c) into main (7cef4dd) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1615      +/-   ##
==========================================
+ Coverage   66.62%   66.70%   +0.08%     
==========================================
  Files          93       93              
  Lines       16607    16583      -24     
  Branches     4402     4398       -4     
==========================================
- Hits        11064    11062       -2     
+ Misses       4402     4383      -19     
+ Partials     1141     1138       -3     
Impacted Files Coverage Δ
cwltool/main.py 71.71% <100.00%> (ø)
cwltool/process.py 88.78% <100.00%> (+1.93%) ⬆️
cwltool/cwltool/job.py 61.78% <0.00%> (-0.80%) ⬇️
cwltool/cwltool/main.py 32.17% <0.00%> (ø)
cwltool/cwltool/process.py 71.49% <0.00%> (+1.31%) ⬆️

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 7cef4dd...f8a5d4c. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Feb 9, 2022

This pull request introduces 1 alert when merging 2e7013f into 7cef4dd - view on LGTM.com

new alerts:

  • 1 for Module-level cyclic import

Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Thanks, Peter. Can we get a test case to prevent regressions?

@lgtm-com
Copy link

lgtm-com bot commented Feb 10, 2022

This pull request introduces 1 alert when merging 9f59f5e into 7cef4dd - view on LGTM.com

new alerts:

  • 1 for Module-level cyclic import

@tetron tetron enabled auto-merge (squash) February 10, 2022 15:59
Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Thanks, I have manually confirmed that your test fails without these fixes. Looks like we have one more area that needs testing

Comment on lines 1127 to 1130
elif e["location"] != ents[basename]["location"]:
raise Exception(
"Conflict between %s and %s", e["location"], ents[basename]["location"]
)
Copy link
Member

Choose a reason for hiding this comment

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

Can we get a test to confirm the conflict detection as well?

@lgtm-com
Copy link

lgtm-com bot commented Feb 10, 2022

This pull request introduces 1 alert when merging f8a5d4c into 7cef4dd - view on LGTM.com

new alerts:

  • 1 for Module-level cyclic import

Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Thanks!

@tetron tetron merged commit 5f93354 into main Feb 10, 2022
@tetron tetron deleted the fix-scandeps branch February 10, 2022 17:15
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