version: redesign output format for the dvc version command #3499 by sahilbhosale63 · Pull Request #4114 · treeverse/dvc

sahilbhosale63 · 2020-06-25T15:55:44Z

Fix #3499 redesign the output format for the dvc version command.

Redesigned the output format for the command: dvc version

…put-dvc Version: Redesign output format treeverse#3499

skshetry · 2020-06-25T16:18:27Z

@sahilbhosale63, thanks for the PR. CI is failing. Did you run pre-commit hooks as mentioned here: https://dvc.org/doc/user-guide/contributing/core?

Also, it'd be good to see the outputs for different DVC installations/repos. Thanks.

skshetry · 2020-06-25T16:24:08Z

-        return ", ".join(supported_remotes)
+        if supported_remotes.__len__() == 9:
+            return "All remotes"
+        elif supported_remotes.__len__() == 1:


In Python, it's not good to use dunder methods directly(__len__). You can just use len().

Done some suggested fixes in code for the dvc command output format issue.

pmrowla

Changes look alright to me for the most part, but there's a couple general issues regarding the new format that I'm not sure about.

When running from a source installation instead of using any of the package/binary install methods (pip install -e ) we output

Build Info: Python 3.7.7 on Darwin-19.5.0-x86_64-i386-64bit installed via None

It might be better for us to just drop the installed via ... part entirely in this case? When I first read None here, it made me think this was a bug in how we detect installation method, rather than thinking it meant "None as in not installed from a binary package".

This was a bit clearer in the old output when we had

Binary: False
Package: None

Also in the new output we do:

Directory cache: apfs on /dev/disk1s1
...
Workspace with apfs on /dev/disk1s1

and it looks a bit strange to have inconsistent formatting between both of these lines (in one line we use a colon and in the other line we use with)

skshetry · 2020-06-29T07:40:29Z

+        if len(supported_remotes) == len(TREES):
+            return "All remotes"
+        elif len(supported_remotes) == 1:
+            return supported_remotes
+        else:
+            return ", ".join(supported_remotes)


@sahilbhosale63, the plan was to make Supports use both remotes and cache types. But, as we didnot go through that, I think it's okay to not go "All remotes" route (i.e what was before with comma separated list of remotes). But, it's your call.

skshetry · 2020-06-29T07:44:21Z

@sahilbhosale63, @pmrowla, maybe we could do something like this:

Build Info: Python 3.7.7 on Darwin-19.5.0-x86_64-i386-64bit (pip)
Build Info: Python 3.7.7 on Darwin-19.5.0-x86_64-i386-64bit (binary)

Also, it would be good to rename Build Info to Platform which is what it is.

Or, could add it to the version: DVC version: 1.0.0 (pip)?

pmrowla · 2020-06-29T07:52:07Z

Or, could add it to the version: DVC version: 1.0.0 (pip)?

This might work best just because the platform line is already going to be pretty long in a lot of cases (even without the installation method)

sahilbhosale63 · 2020-07-21T04:00:41Z

@skshetry can you please review this patch?

skshetry

I am really sorry for taking it so long to review, looks good in general, just a few minor issues.

skshetry · 2020-07-21T04:24:11Z

-            f"Binary: {is_binary()}",
-            f"Package: {PKG}",
-            f"Supported remotes: {self.get_supported_remotes()}",
+            f"DVC version: {__version__} ({PKG})",


I get:

DVC version: 1.0.1+412f39 (None)

PKG can be None too. So, let's leave that out if it's None.

skshetry · 2020-07-21T04:24:48Z

            if os.path.exists(repo.cache.local.cache_dir):
                info.append(
-                    "Cache: {}".format(self.get_linktype_support_info(repo))
+                    "Cache types: " + self.get_linktype_support_info(repo)


Let's use {} formated string as above with just the addtion of Cache types:.

skshetry · 2020-07-21T04:28:20Z

        return "dvc (subdir), git"

-    return "dvc, git"
+    return "dvc + git"


We need to make this uniform, either we should make it all commas or all plus-es. Could just use comma for now.

Same for cache types which seems to use / right now.

+1 for commas (since / is a path separator)
+1 to check other occurrences

sahilbhosale63 · 2020-07-21T15:29:21Z

@skshetry @pmrowla @jorgeorpinel Does this looks good or should I make some changes?

jorgeorpinel · 2020-07-22T03:37:46Z

I just noticed this PR, thanks for the mention.

Supports: All remotes

How about

Supported remotes: {List them always} — actually I think that's how it is now so maybe this doesn't need a change...

Also:

Cache types -> Supported cache types ?
Workspace Directory - lower case directory
Cache Directory - same ^. And put after Workspace dir?
Repo: -> Repo control: ?

Also keep in mind this may need updates to https://dvc.org/doc/command-reference/version or other docs after merge. Thanks

shcheklein · 2020-07-22T03:41:41Z

@sahilbhosale63 why do we need (----------------) separator?

How does the output looks like when no cache dir is available?
I don't see the install method ... is it None on your screenshot? should we use some human readable name?

sahilbhosale63 · 2020-07-22T03:59:13Z

@sahilbhosale63 why do we need (----------------) separator?

How does the output looks like when no cache dir is available?
I don't see the install method ... is it None on your screenshot? should we use some human readable name?

@shcheklein When there is no cache dir it gives a warning. We have dicussed about this previously here #3499 (comment)

The PKG is (None) because here in this case we have not installed dvc using pip or any other package manager.

Yes, we need that seperator as decided here #3499

sahilbhosale63 · 2020-07-22T08:40:17Z

@jorgeorpinel Added #3499 issue number to the PR description with the Fix keyword.

sahilbhosale63 · 2020-07-23T02:54:21Z

The Travis build test failures are unrelated to the changes which I have made.

skshetry

LGTM, just not sure about All remotes, but fine for now.

pmrowla

LGTM

Co-authored-by: Saugat Pachhai <suagatchhetri@outlook.com>

efiop · 2020-07-28T17:29:32Z

Thank you @sahilbhosale63 ! 🙏

jorgeorpinel · 2020-07-29T03:18:42Z

Created treeverse/dvc.org/issues/1635 to update the docs.

sahilbhosale63 added 2 commits June 25, 2020 19:54

Version: Redesign output format treeverse#3499

365713e

Redesigned the output format for the command: dvc version

Merge pull request #1 from sahilbhosale63/sahilbhosale63-reformat-out…

361f562

…put-dvc Version: Redesign output format treeverse#3499

efiop requested a review from skshetry June 25, 2020 16:00

skshetry reviewed Jun 25, 2020

View reviewed changes

Comment thread dvc/command/version.py Outdated

skshetry reviewed Jun 25, 2020

View reviewed changes

Comment thread dvc/command/version.py Outdated

sahilbhosale63 added 3 commits June 26, 2020 22:26

version: reformat output format treeverse#3499

d68ddb2

Done some suggested fixes in code for the dvc command output format issue.

version: redesign output format treeverse#3499

96add40

version: redesign output format treeverse#3499

e9cb620

pmrowla suggested changes Jun 27, 2020

View reviewed changes

Comment thread dvc/command/version.py Outdated

version: redesign output format for dvc command treeverse#3499

7f66772

sahilbhosale63 requested a review from pmrowla June 28, 2020 11:24

pmrowla reviewed Jun 29, 2020

View reviewed changes

Comment thread dvc/command/version.py Outdated

pmrowla reviewed Jun 29, 2020

View reviewed changes

Comment thread dvc/command/version.py Outdated

pmrowla suggested changes Jun 29, 2020

View reviewed changes

skshetry reviewed Jun 29, 2020

View reviewed changes

weekly-digest Bot mentioned this pull request Jul 5, 2020

Weekly Digest (28 June, 2020 - 5 July, 2020) #4168

Closed

sahilbhosale63 added 2 commits July 11, 2020 13:02

version: redesign output format treeverse#3499

6064659

version: redesign output format treeverse#3499

412f39d

weekly-digest Bot mentioned this pull request Jul 12, 2020

Weekly Digest (5 July, 2020 - 12 July, 2020) #4195

Closed

efiop requested a review from skshetry July 17, 2020 02:38

weekly-digest Bot mentioned this pull request Jul 19, 2020

Weekly Digest (12 July, 2020 - 19 July, 2020) #4238

Closed

skshetry reviewed Jul 21, 2020

View reviewed changes

Comment thread dvc/command/version.py

This comment has been minimized.

Sign in to view

sahilbhosale63 added 2 commits July 23, 2020 07:09

Redesign output format for the dvc version command treeverse#3499

4e55522

Redesign output format for the dvc version command treeverse#3499

753efe7

weekly-digest Bot mentioned this pull request Jul 26, 2020

Weekly Digest (19 July, 2020 - 26 July, 2020) #4285

Closed

skshetry reviewed Jul 28, 2020

View reviewed changes

Comment thread dvc/command/version.py Outdated

skshetry reviewed Jul 28, 2020

View reviewed changes

skshetry requested review from efiop, pared and pmrowla July 28, 2020 11:52

pmrowla approved these changes Jul 28, 2020

View reviewed changes

pared approved these changes Jul 28, 2020

View reviewed changes

Update dvc/command/version.py

999c9e1

Co-authored-by: Saugat Pachhai <suagatchhetri@outlook.com>

efiop merged commit f4595a0 into treeverse:master Jul 28, 2020

jorgeorpinel mentioned this pull request Jul 29, 2020

cmd: update version ref. treeverse/dvc.org#1635

Closed

weekly-digest Bot mentioned this pull request Aug 2, 2020

Weekly Digest (26 July, 2020 - 2 August, 2020) #4315

Closed

Conversation

sahilbhosale63 commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skshetry commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skshetry Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pmrowla left a comment

Choose a reason for hiding this comment

Uh oh!

skshetry Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skshetry commented Jun 29, 2020

Uh oh!

pmrowla commented Jun 29, 2020

Uh oh!

sahilbhosale63 commented Jul 21, 2020

Uh oh!

skshetry left a comment

Choose a reason for hiding this comment

Uh oh!

skshetry Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

skshetry Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skshetry Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

jorgeorpinel Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

shcheklein Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sahilbhosale63 commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorgeorpinel commented Jul 22, 2020

Uh oh!

shcheklein commented Jul 22, 2020

Uh oh!

This comment has been minimized.

sahilbhosale63 commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

sahilbhosale63 commented Jul 22, 2020 • edited by jorgeorpinel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sahilbhosale63 commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

skshetry left a comment

Choose a reason for hiding this comment

Uh oh!

pmrowla left a comment

Choose a reason for hiding this comment

Uh oh!

efiop commented Jul 28, 2020

Uh oh!

jorgeorpinel commented Jul 29, 2020

Uh oh!

Reviewers

Assignees

Labels

sahilbhosale63 commented Jun 25, 2020 •

edited

Loading

skshetry commented Jun 25, 2020 •

edited

Loading

skshetry Jun 29, 2020 •

edited

Loading

sahilbhosale63 commented Jul 21, 2020 •

edited

Loading

sahilbhosale63 commented Jul 22, 2020 •

edited

Loading

sahilbhosale63 commented Jul 22, 2020 •

edited by jorgeorpinel

Loading

sahilbhosale63 commented Jul 23, 2020 •

edited

Loading