Skip to content

use SI prefixes for progress#2817

Merged
efiop merged 1 commit into
treeverse:masterfrom
casperdcl:si-progress
Nov 21, 2019
Merged

use SI prefixes for progress#2817
efiop merged 1 commit into
treeverse:masterfrom
casperdcl:si-progress

Conversation

@casperdcl
Copy link
Copy Markdown
Contributor

@casperdcl casperdcl commented Nov 19, 2019 •
edited
Loading

changes 560000/1000023 in progress bars to 560k/1M

Note that this looses some precision for large numbers but looks nicer. Not sure if this is a good or bad thing.

asciicast

@casperdcl casperdcl added enhancement Enhances DVC feature request Requesting a new feature p3-nice-to-have It should be done this or next sprint c1-quick-fix ui user interface / interaction discussion requires active participation to reach a conclusion labels Nov 19, 2019
@casperdcl casperdcl requested a review from efiop November 19, 2019 20:32
@casperdcl casperdcl self-assigned this Nov 19, 2019
@efiop
Copy link
Copy Markdown
Contributor

efiop commented Nov 20, 2019

@casperdcl Sure it is a great thing, who wants the byte precision anyway? 😄

efiop
efiop approved these changes Nov 20, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Neat!

@efiop
Copy link
Copy Markdown
Contributor

efiop commented Nov 20, 2019

Is this patch all we need for all progress bars that we have? I mean, no additional modifications required for any other bars, right? Just making sure.

@efiop efiop requested review from a user and pared November 20, 2019 00:57
@casperdcl
Copy link
Copy Markdown
Contributor Author

casperdcl commented Nov 20, 2019

Yes nothing else needed.

who wants the byte precision anyway?

It also reduces the file count precision when there are many files.

@efiop
Copy link
Copy Markdown
Contributor

efiop commented Nov 20, 2019

It also reduces the file count precision when there are many files.

@casperdcl Oh... How would that look now? Could you show us? Also, what are your thoughts on that from UI perspective?

@casperdcl
Copy link
Copy Markdown
Contributor Author

casperdcl commented Nov 21, 2019

@efiop before/after recording added to description.

what are your thoughts on that from UI perspective?

I think in certain circumstances users may prefer granular file counts over SI prefixes.

@efiop
Copy link
Copy Markdown
Contributor

efiop commented Nov 21, 2019

Well, on the other hand, when talking about number of files, we always have complimentary progress bars for md5 computations and whatnot (and there will be additional coming) so, it is not like dvc will be looking frozen. So I think we can afford taking this behavior as a standard. Could consider a flag for granular output if someone asks for it.

@efiop efiop merged commit 5c5d1ed into treeverse:master Nov 21, 2019
@casperdcl casperdcl deleted the si-progress branch November 21, 2019 23:52
@weekly-digest weekly-digest Bot mentioned this pull request Nov 24, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@efiop efiop efiop approved these changes

@pared pared pared approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@casperdcl casperdcl

Labels

discussion requires active participation to reach a conclusion enhancement Enhances DVC feature request Requesting a new feature p3-nice-to-have It should be done this or next sprint ui user interface / interaction

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@casperdcl @efiop @pared