Skip to content
Merged
26 changes: 20 additions & 6 deletions dvc/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from voluptuous import Schema, Required, Invalid

from dvc.repo import Repo
from dvc.exceptions import DvcException
from dvc.exceptions import DvcException, NotDvcRepoError
from dvc.external_repo import external_repo


Expand Down Expand Up @@ -43,13 +43,27 @@ class SummonError(DvcException):
pass


class UrlNotDvcRepoError(DvcException):
"""Thrown if given url is not a DVC repository.

Args:
url (str): url to the repository.
"""

def __init__(self, url):
super().__init__("URL '{}' is not a dvc repository.".format(url))


def get_url(path, repo=None, rev=None, remote=None):
"""Returns an url of a resource specified by path in repo"""
with _make_repo(repo, rev=rev) as _repo:
abspath = os.path.join(_repo.root_dir, path)
out, = _repo.find_outs_by_path(abspath)
remote_obj = _repo.cloud.get_remote(remote)
return str(remote_obj.checksum_to_path_info(out.checksum))
try:
with _make_repo(repo, rev=rev) as _repo:
abspath = os.path.join(_repo.root_dir, path)
out, = _repo.find_outs_by_path(abspath)
remote_obj = _repo.cloud.get_remote(remote)
return str(remote_obj.checksum_to_path_info(out.checksum))
except NotDvcRepoError:
raise UrlNotDvcRepoError(repo)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll raise this same error in _make_repo in a separate PR. Just keeping it here for now.



def open(path, repo=None, rev=None, remote=None, mode="r", encoding=None):
Expand Down
25 changes: 25 additions & 0 deletions dvc/command/get.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,27 @@


class CmdGet(CmdBaseNoRepo):
def _show_url(self):
from dvc.api import get_url

try:
url = get_url(
self.args.path, repo=self.args.url, rev=self.args.rev
)
logger.info(url)
except DvcException:
logger.exception("failed to show url")
Copy link
Contributor

@jorgeorpinel jorgeorpinel Jan 20, 2020

Choose a reason for hiding this comment

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

Capitalization: "Failed to show URL" Maybe end in period . ?
Depends on whether it's printed as part of a larger message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jorgeorpinel, the lowercase seems to be used everywhere in the codebase. And, because this is a logger.exception, our formatter handles adding extra information from raised exception besides this particular logging message. Notice the messages on following examples:

➜ dvc get https://github.com/schacon/cowsay install.sh --show-url 
ERROR: failed to show url - URL 'https://github.com/schacon/cowsay' is not a dvc repository.
➜ dvc get . dir/file --show-url
ERROR: failed to show url - unable to find DVC-file with output 'dir/file'

Copy link
Member

Choose a reason for hiding this comment

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

failed to show url - URL it looks strange though :) First it proves that the lowercase seems to be used everywhere is not true (and it should not be, typically, acronyms and initialisms are written in all capital letters to distinguish them from ordinary words. - NASA, URL, DVC, etc). Second, you have a repetition here. Repetition is probably fine if reasons vary and you can't actually control them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@shcheklein, it's not something that I add, but, I'm utilizing the fact that something will be there. So, it's more like "(what failed) - (the reason)".

Copy link
Collaborator Author

@skshetry skshetry Jan 21, 2020

Choose a reason for hiding this comment

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

And, yes, regarding lowercase, I just looked into it and it's not everywhere for other cases of logging, but for exceptions, lowercase is used everywhere.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jan 23, 2020

Choose a reason for hiding this comment

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

because this is a logger.exception, our formatter handles adding extra information

Thanks, good to know this.

lowercase seems to be used everywhere is not true...

➕ 1️⃣ No need to use bad grammar on purpose because of existing bad grammar.

failed to show url - URL it looks strange though :) ...

it's more like "(what failed) - (the reason)".

So then the reason part in this case should start directly with http://... – but that's not part of this PR...

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jan 23, 2020

Choose a reason for hiding this comment

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

the reason part in this case should start directly with http://... – but that's not part of this PR

OK I changed this in ec0c39a#diff-3b2745249f7139953ab4b45b382e69e9 but this whole conversation kind of opened a can of worms and that PR (#3220) grew quite a bit 😅 Hoping core team doesn't flip out.

but for exceptions, lowercase is used everywhere

Thanks for the tip. I may review some of these too 😋

return 1

return 0

def run(self):
if self.args.show_url:
return self._show_url()

return self._get_file_from_repo()

def _get_file_from_repo(self):
from dvc.repo import Repo

try:
Expand Down Expand Up @@ -55,4 +75,9 @@ def add_parser(subparsers, parent_parser):
get_parser.add_argument(
"--rev", nargs="?", help="Git revision (e.g. branch, tag, SHA)"
)
get_parser.add_argument(
"--show-url",
action="store_true",
help="Returns path/url to the location in remote for specified path",
Copy link
Contributor

@efiop efiop Jan 17, 2020

Choose a reason for hiding this comment

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

Could use the description from https://github.com/iterative/dvc/pull/3130/files

Copy link
Contributor

Choose a reason for hiding this comment

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

Though, looks like it is still up for discussion, so i'm sure we will modify it while working on the docs. Let's keep it as is for now 👍

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jan 20, 2020

Choose a reason for hiding this comment

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

Ah yeah I would change this description, its confusing. Sorry, didn't notice before merge...

This comment was marked as resolved.

)
get_parser.set_defaults(func=CmdGet)
12 changes: 11 additions & 1 deletion tests/func/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import pytest

from dvc import api
from dvc.api import SummonError
from dvc.api import SummonError, UrlNotDvcRepoError
from dvc.compat import fspath
from dvc.exceptions import FileMissingError
from dvc.main import main
Expand Down Expand Up @@ -51,6 +51,16 @@ def test_get_url_external(erepo_dir, remote_url):
assert api.get_url("foo", repo=repo_url) == expected_url


def test_get_url_git_only_repo_throws_exception(tmp_dir, scm):
tmp_dir.scm_gen({"foo": "foo"}, commit="initial")

with pytest.raises(UrlNotDvcRepoError) as exc_info:
api.get_url("foo", fspath(tmp_dir))

# On windows, `exc_info` has path escaped, eg: `C:\\\\Users\\\\travis`
assert fspath(tmp_dir) in str(exc_info).replace("\\\\", "\\")


@pytest.mark.parametrize("remote_url", all_remote_params, indirect=True)
def test_open(remote_url, tmp_dir, dvc):
run_dvc("remote", "add", "-d", "upstream", remote_url)
Expand Down
28 changes: 28 additions & 0 deletions tests/func/test_get.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

from dvc.cache import Cache
from dvc.config import Config
from dvc.main import main
from dvc.repo.get import GetDVCFileError, PathMissingError
from dvc.repo import Repo
from dvc.system import System
Expand Down Expand Up @@ -184,3 +185,30 @@ def test_get_from_non_dvc_master(tmp_dir, erepo_dir, caplog):

assert caplog.text == ""
assert (tmp_dir / dst).read_text() == "some_contents"


def test_get_url_positive(tmp_dir, erepo_dir, caplog):
with erepo_dir.chdir():
erepo_dir.dvc_gen("foo", "foo")

caplog.clear()
with caplog.at_level(logging.ERROR, logger="dvc"):
assert main(["get", fspath(erepo_dir), "foo", "--show-url"]) == 0
assert caplog.text == ""


def test_get_url_not_existing(tmp_dir, erepo_dir, caplog):
with caplog.at_level(logging.ERROR, logger="dvc"):
assert (
main(["get", fspath(erepo_dir), "not-existing-file", "--show-url"])
== 1
)
assert "failed to show url" in caplog.text


def test_get_url_git_only_repo(tmp_dir, scm, caplog):
tmp_dir.scm_gen({"foo": "foo"}, commit="initial")

with caplog.at_level(logging.ERROR):
assert main(["get", fspath(tmp_dir), "foo", "--show-url"]) == 1
assert "failed to show url" in caplog.text
15 changes: 15 additions & 0 deletions tests/unit/command/test_get.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,18 @@ def test_get(mocker):
assert cmd.run() == 0

m.assert_called_once_with("repo_url", path="src", out="out", rev="version")


def test_get_url(mocker, caplog):
cli_args = parse_args(
["get", "repo_url", "src", "--rev", "version", "--show-url"]
)
assert cli_args.func == CmdGet

cmd = cli_args.func(cli_args)
m = mocker.patch("dvc.api.get_url", return_value="resource_url")

assert cmd.run() == 0
assert "resource_url" in caplog.text

m.assert_called_once_with("src", repo="repo_url", rev="version")