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

BF: take path from SSHRI, test URLs not only on Windows #5881

Merged
merged 1 commit into from
Aug 11, 2021

Conversation

yarikoptic
Copy link
Member

While extending test with SSH RIs I realized that

(a + b if False else d) == d, so in effect those example URLs
were tested only on Windows. I have made it a bit more explicit so
we do not fall into this trap again.

Closes #5880

  • I am really not sure yet how we haven't ran into this bug before, since cloning over ssh while resorting to a default name should be a common task.
  • I think that we could further improve _get_installationpath_from_url to avoid any class checks. Logic should not rely on the class, but rather on either path is provided IMHO. I might submit a follow up PR against master, wanted to keep this PR to the point

While extending test with SSH RIs I realized that

a + b if False else d == d, so in effect those example URLs
were tested only on Windows.  I have made it a bit more explicit so
we do not fall into this trap again.

Closes datalad#5880
@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Aug 9, 2021
yarikoptic added a commit to yarikoptic/datalad that referenced this pull request Aug 9, 2021
Would rely on RI to have non-degenerating .path, or .hostname, and only
then (ab)use entire URL as path, taking the last component

A complimentary/alternative to a bug fix for SSH in
datalad#5881
and sits on top of it
@codecov
Copy link

codecov bot commented Aug 9, 2021

Codecov Report

Merging #5881 (8e3eafb) into maint (8004a5e) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5881      +/-   ##
==========================================
- Coverage   90.34%   90.32%   -0.02%     
==========================================
  Files         300      300              
  Lines       42395    42396       +1     
==========================================
- Hits        38301    38294       -7     
- Misses       4094     4102       +8     
Impacted Files Coverage Δ
datalad/core/distributed/clone.py 91.62% <100.00%> (ø)
datalad/core/distributed/tests/test_clone.py 97.37% <100.00%> (+<0.01%) ⬆️
datalad/downloaders/tests/test_http.py 89.17% <0.00%> (-1.89%) ⬇️

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 8004a5e...8e3eafb. Read the comment docs.

@bpoldrack bpoldrack merged commit 2d53dd1 into datalad:maint Aug 11, 2021
@yarikoptic yarikoptic deleted the bf-clone-url-path branch August 18, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants