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

diff: support targets #3388

Closed
DavidGOrtega opened this issue Feb 23, 2020 · 11 comments · Fixed by #4938
Closed

diff: support targets #3388

DavidGOrtega opened this issue Feb 23, 2020 · 11 comments · Fixed by #4938
Assignees
Labels
feature request Requesting a new feature p2-medium Medium priority, should be done, but less important product: VSCode Integration with VSCode extension

Comments

@DavidGOrtega
Copy link

dvc diff -t models HEAD^ HEAD
ERROR: unrecognized arguments: -t HEAD

Please provide information about your setup
Ubuntu 18.04
Dvc 0.84.0

@triage-new-issues triage-new-issues bot added the triage Needs to be triaged label Feb 23, 2020
@efiop efiop added the feature request Requesting a new feature label Feb 23, 2020
@triage-new-issues triage-new-issues bot removed the triage Needs to be triaged label Feb 23, 2020
@efiop efiop changed the title dvc diff target unrecognised diff: support targets Feb 23, 2020
@efiop efiop added the p1-important Important, aka current backlog of things to do label Feb 23, 2020
@dmpetrov
Copy link
Member

It is a good feature idea but probably not a critical one.

@DavidGOrtega I'd appreciate it if you provide more context to understand how critical the issue is?

@DavidGOrtega
Copy link
Author

@dmpetrov I tried it according to the docs and I found that issue. For ci is not an issue aside of having more features.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Feb 24, 2020

This feature (--target option) was removed in #3229. Do we remember why? 🙂 Cc @efiop

@dmpetrov
Copy link
Member

@jorgeorpinel as far as I remember it was removed for two reasons: it was not a priority and it seems fine not to have it because git diff does not support any targets.

Is there a good use case when we need targets (except backward compatibility)? It is important to understand the priority.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Feb 25, 2020

Ohhh I see. I think this issue is mainly about a discrepancy between dvc diff and its cmd ref in https://dvc.org/doc/command-reference/diff so we can probably transfer it to iterative/dvc.org ?

In fact I thought it was addressed in iterative/dvc.org/pull/953 but I see not. Lmk if you guys agree and I'll transfer it so it's addressed ASAP.

UPDATE: Should we close this issue? The docs are up to date now, actually.

@shcheklein
Copy link
Member

@dmpetrov

because git diff does not support any targets

Looks like it does.

git diff HEAD^ -- dvc/api.py

returns:

diff --git a/dvc/api.py b/dvc/api.py
index def9e8cf..d1312baa 100644
--- a/dvc/api.py
+++ b/dvc/api.py
@@ -18,10 +18,10 @@ class UrlNotDvcRepoError(DvcException):


 def get_url(path, repo=None, rev=None, remote=None):
-    """
-    Returns the full URL to the data artifact specified by its `path` in a
-    `repo`.
-    NOTE: There is no guarantee that the file actually exists in that location.
+    """Returns URL to the storage location of a data artifact tracked
+    by DVC, specified by its path in a repo.
+
+    NOTE: There's no guarantee that the file actually exists in that location.

https://git-scm.com/docs/git-diff

@shcheklein

This comment has been minimized.

@jorgeorpinel

This comment has been minimized.

@shcheklein
Copy link
Member

Let's keep it. This is a pretty relevant feature. Especially considering that there was some confusion around git diff.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Feb 25, 2020

OK

Looks like (dvc diff) does (support targets).
git diff HEAD^ -- dvc/api.py

You don't even need --, so if we're re-implementing this, maybe just make it an implicit (and optional) [targets [targets]] cmd param like in fetch/pull/push.

@efiop efiop added p2-medium Medium priority, should be done, but less important and removed p1-important Important, aka current backlog of things to do labels Feb 28, 2020
@dmpetrov dmpetrov added the product: VSCode Integration with VSCode extension label Nov 20, 2020
@sandeepmistry
Copy link
Contributor

I've started a draft pull request on this if anyone would like to give early feedback the changes: #4938

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Requesting a new feature p2-medium Medium priority, should be done, but less important product: VSCode Integration with VSCode extension
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants