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

ENH: downloaders: Recommend requests_ftp on ftp failure #4788

Merged
merged 1 commit into from Jul 31, 2020

Conversation

kyleam
Copy link
Collaborator

@kyleam kyleam commented Jul 30, 2020

If a download of an ftp:// URL fails and requests_ftp is not installed, mention requests_ftp in the error message, as suggested by @nicholsn in gh-4787.

@codecov
Copy link

codecov bot commented Jul 30, 2020

Codecov Report

Merging #4788 into maint will increase coverage by 0.20%.
The diff coverage is 91.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #4788      +/-   ##
==========================================
+ Coverage   89.40%   89.60%   +0.20%     
==========================================
  Files         288      287       -1     
  Lines       40280    40269      -11     
==========================================
+ Hits        36013    36085      +72     
+ Misses       4267     4184      -83     
Impacted Files Coverage Δ
datalad/core/distributed/tests/test_push.py 97.78% <ø> (-0.06%) ⬇️
datalad/distribution/create_sibling_github.py 83.87% <ø> (ø)
datalad/interface/run_procedure.py 90.79% <ø> (ø)
datalad/support/annexrepo.py 86.76% <0.00%> (+0.33%) ⬆️
datalad/support/archive_utils_7z.py 88.00% <ø> (ø)
datalad/support/exceptions.py 83.41% <0.00%> (-0.44%) ⬇️
datalad/downloaders/http.py 81.85% <20.00%> (-0.90%) ⬇️
datalad/support/tests/test_gitrepo.py 99.77% <75.00%> (ø)
datalad/tests/test_utils.py 95.50% <87.50%> (-0.09%) ⬇️
datalad/support/gitrepo.py 90.54% <96.87%> (+0.17%) ⬆️
... and 29 more

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 f7f15b9...8a75a74. Read the comment docs.

@yarikoptic
Copy link
Member

yarikoptic commented Jul 30, 2020

Looks good to me, but shouldn't we position it against maint?

@kyleam
Copy link
Collaborator Author

kyleam commented Jul 30, 2020

but shouldn't we position it against maint?

I don't view it as a bug fix.

@vsoch
Copy link
Collaborator

vsoch commented Jul 30, 2020

@kyleam note that @yarikoptic also requested the contributors update to go to maint (and not master), and that might better be against master since it wouldn't be a bug fix, but just an update to the contributors badge/metadata in the readme.

@yarikoptic
Copy link
Member

yarikoptic commented Jul 31, 2020

but shouldn't we position it against maint?

I don't view it as a bug fix.

The change is trivial and could improve UX of some users at minimal cost/risk... I would have preferred it in maint so it has a better chance to see the light of being included in a release. But if you feel strongly, I am ok either way, and please merge at your will.

@kyleam
Copy link
Collaborator Author

kyleam commented Jul 31, 2020

The change is trivial and could improve UX of some users at minimal cost/risk... I would have preferred it in maint so it has a better chance to see the light of being included in a release.

Based on past discussions (in particular what came out of #4238 and related discussions in PRs like 4382), I think we should lean towards master for these sorts of things. (Yes, what counts as a pure bug fix isn't always clear cut, and some meta things like testing infrastructure changes make sense to go to maint.) Much of the motivation for this is keeping the focus on master.

But, judging by gh-4790, @mih may be on board with the "make it into a release sooner" motivation for targeting maint. So, I'll rebase this PR to maint and will recalibrate for future PRs.

Downloading ftp:// URLs requires requests_ftp, which is an optional
dependency.  If a download of an ftp:// URL fails and requests_ftp is
not installed, mention requests_ftp in the error message.

Closes datalad#4787.

Suggested-by: Nolan Nichols <nnichols@mazetx.com>
@kyleam kyleam changed the base branch from master to maint Jul 31, 2020
@kyleam
Copy link
Collaborator Author

kyleam commented Jul 31, 2020

Test failures are unrelated. I know at least one has an open issue, but I will check if the others do.

@kyleam kyleam merged commit a4bfd9f into datalad:maint Jul 31, 2020
1 of 2 checks passed
@kyleam kyleam deleted the ftp-better-error branch Jul 31, 2020
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.

None yet

3 participants