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

import-url: support directories #2894

Merged
merged 14 commits into from
Dec 6, 2019
Merged

Conversation

verasativa
Copy link
Contributor

@verasativa verasativa commented Dec 5, 2019

No description provided.

@efiop efiop changed the title feat: [WIP] remote dir download [WIP] import-url: support directories Dec 5, 2019
dvc/remote/base.py Outdated Show resolved Hide resolved
dvc/remote/base.py Outdated Show resolved Hide resolved
dvc/remote/base.py Outdated Show resolved Hide resolved
dvc/remote/base.py Outdated Show resolved Hide resolved
dvc/remote/base.py Outdated Show resolved Hide resolved
Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Looks great @verasativa ! 🎉 Please see a few minor comments up above.

Co-Authored-By: Ruslan Kuprieiev <kupruser@gmail.com>
@efiop
Copy link
Contributor

efiop commented Dec 5, 2019

@verasativa One more thing. Let's add a simple test for this. Take a look at tests/unit/remote/test_remote_dir.py , where we have all of the related code. The test would look something like:

@pytest.mark.parametrize("remote", remotes, indirect=True)
def test_download_dir(remote, tmpdir):
    path = fspath(tmpdir / "data")
    to_info = PathInfo(path)
    remote.download(remote.path_info / "data", to_info)
    assert os.path.isdir(path)
    # check the list of files

verasativa and others added 2 commits December 5, 2019 16:45
Co-Authored-By: Ruslan Kuprieiev <kupruser@gmail.com>
dvc/remote/base.py Outdated Show resolved Hide resolved
dvc/remote/base.py Outdated Show resolved Hide resolved
dvc/remote/base.py Outdated Show resolved Hide resolved
dvc/remote/base.py Outdated Show resolved Hide resolved
dvc/remote/base.py Outdated Show resolved Hide resolved
dvc/remote/base.py Outdated Show resolved Hide resolved
dvc/remote/base.py Outdated Show resolved Hide resolved
dvc/remote/base.py Outdated Show resolved Hide resolved
dvc/remote/base.py Outdated Show resolved Hide resolved
dvc/remote/base.py Outdated Show resolved Hide resolved
@@ -1,5 +1,6 @@
# -*- coding: utf-8 -*-
import pytest
import os

from dvc.remote.s3 import RemoteS3

Copy link
Contributor

@efiop efiop Dec 6, 2019

Choose a reason for hiding this comment

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

The reason why tests are failing 🙂

Suggested change
from dvc.utils import walk_files

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

@verasativa Looks great! Just one more minor comment up above and I think we will be done.

dvc/remote/base.py Show resolved Hide resolved
@pared pared self-requested a review December 6, 2019 13:47
@pared
Copy link
Contributor

pared commented Dec 6, 2019

Ok, LGTM after @efiop suggestions are resolved

@efiop
Copy link
Contributor

efiop commented Dec 6, 2019

For the record: 1 windows test failed because of temporary issues with chocolatey, that are unrelated to this patch.

@verasativa verasativa changed the title [WIP] import-url: support directories import-url: support directories Dec 6, 2019
Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Thank you so much! 🎉

@efiop efiop merged commit 8fa2682 into iterative:master Dec 6, 2019
@efiop
Copy link
Contributor

efiop commented Dec 6, 2019

For the record: no need to adjust docs, as they already say that we support files and dirs.

EDIT: actually we do need to adjust them, see linked issue below.

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.

4 participants