Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 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 @@ -108,12 +108,16 @@ def read(path, repo=None, rev=None, remote=None, mode="r", encoding=None):


@contextmanager
def _make_repo(repo_url, rev=None):
if rev is None and (not repo_url or os.path.exists(repo_url)):
yield Repo(repo_url)
else:
with external_repo(url=repo_url, rev=rev) as repo:
yield repo
def _make_repo(repo_url=None, rev=None):
repo_url = repo_url or os.getcwd()
if rev is None and os.path.exists(repo_url):
try:
yield Repo(repo_url)
return
except NotDvcRepoError:
pass # fallthrough to external_repo
with external_repo(url=repo_url, rev=rev) as repo:
yield repo


def summon(name, repo=None, rev=None, summon_file="dvcsummon.yaml", args=None):
Expand Down
4 changes: 2 additions & 2 deletions dvc/external_repo.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,10 @@ def pull_to(self, path, to_info):
raise PathMissingError(path, self.url)

@contextmanager
def open_by_relpath(self, path, mode="r", encoding=None):
def open_by_relpath(self, path, mode="r", encoding=None, **kwargs):
Copy link
Collaborator Author

@skshetry skshetry Jan 23, 2020

Choose a reason for hiding this comment

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

open_by_relpath on the Repo also takes more arguments such as remote, etc which does not make sense here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it works it should stay as is. Let's not add ignored arguments, this will lead to silent errors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does not as it also gets unexpected arguments from Repo.open_by_relpath:
https://github.com/iterative/dvc/blob/49039968f74282bdab3aa05b4582e5d392fc0ccc/dvc/repo/__init__.py#L430

Copy link
Collaborator Author

@skshetry skshetry Jan 23, 2020

Choose a reason for hiding this comment

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

We could though add remote, or not even send one, but, I prefer this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand. This line is never called from Repo.open_by_relpath().

Copy link
Collaborator Author

@skshetry skshetry Jan 23, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, then we should add only remote param not **kwargs and assert remote is None. This way things won't be silently ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Suor assert is not needed, as the point of that method is to provide the same interface for git and dvc repos, so it should be silently ignored there. Or am I missing something?

try:
abs_path = os.path.join(self.root_dir, path)
with open(abs_path, mode, encoding) as fd:
with open(abs_path, mode, encoding=encoding) as fd:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Third argument is buffering.

yield fd
except FileNotFoundError:
raise PathMissingError(path, self.url)
Expand Down
4 changes: 2 additions & 2 deletions tests/func/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from dvc import api
from dvc.api import SummonError, UrlNotDvcRepoError
from dvc.compat import fspath
from dvc.exceptions import FileMissingError, NotDvcRepoError
from dvc.exceptions import FileMissingError
from dvc.main import main
from dvc.path_info import URLInfo
from dvc.remote.config import RemoteConfig
Expand Down Expand Up @@ -54,7 +54,7 @@ def test_get_url_external(erepo_dir, remote_url):
def test_get_url_requires_dvc(tmp_dir, scm):
tmp_dir.scm_gen({"foo": "foo"}, commit="initial")

with pytest.raises(NotDvcRepoError, match="not inside of a DVC repo"):
with pytest.raises(UrlNotDvcRepoError, match="not a DVC repository"):
api.get_url("foo", repo=fspath(tmp_dir))

with pytest.raises(UrlNotDvcRepoError):
Expand Down