New file progress #933
New file progress #933
Conversation
asvetlov
commented
Jul 29, 2019
asvetlov
commented
- Rewrite CLI progress for file operations
- Enable the progress by default on TTY
|
The code is mainly finished, I like how it looks now. |
neuromation/api/storage.py
Outdated
| for child in await self.ls(src): | ||
| progress.enter(StorageProgressEnterDir(src, dst)) | ||
| folder = await self.ls(src) | ||
| folder.sort(key=lambda item: (item.is_dir(), item.name)) |
There was a problem hiding this comment.
The same sorting key is used in upload_dir() and download_dir(). Maybe expose it as a named function?
There was a problem hiding this comment.
Quite not the same: item is FileStatus here but pathlib.Path in other places.
Codecov Report
@@ Coverage Diff @@
## master #933 +/- ##
==========================================
+ Coverage 91.29% 91.36% +0.06%
==========================================
Files 37 37
Lines 4103 4203 +100
Branches 608 634 +26
==========================================
+ Hits 3746 3840 +94
Misses 248 248
- Partials 109 115 +6
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #933 +/- ##
==========================================
+ Coverage 91.29% 91.76% +0.46%
==========================================
Files 37 37
Lines 4103 4201 +98
Branches 608 633 +25
==========================================
+ Hits 3746 3855 +109
+ Misses 248 237 -11
Partials 109 109
Continue to review full report at Codecov.
|
| return ( | ||
| self.color_indicator[GnuIndicators.LEFT] | ||
| + color | ||
| + self.color_indicator[GnuIndicators.RIGHT] | ||
| + underline |
There was a problem hiding this comment.
Why not use style(lable, fg=color, underline=self._underline or None)?
There was a problem hiding this comment.
It produces a little different ANSI sequence.
I've tried but rejected the idea for the sake of less unittest changes.
| @@ -43,17 +42,16 @@ def __init__(self) -> None: | |||
| def total_lines(self) -> int: | |||
| return self._total_lines | |||
|
|
|||
| def print(self, text: str, lineno: Optional[int] = None) -> str: | |||
| def print(self, text: str, lineno: int = -1) -> str: | |||
There was a problem hiding this comment.
Seems lineno was 1-based, but now it is 0-based. Is it correct?
There was a problem hiding this comment.
Yes, this is true.
I've found that 0-based index is more native for Python program.
Also, -1 is a good indicator for not set value here, it is better than None from typing perspective.
The change doesn't touch the public part of API.
| width = self.full_width | ||
| while len(label) > width: | ||
| parts = list(url.parts) | ||
| if len(parts) > 1: |
There was a problem hiding this comment.
Would not be better to check the negative condition and get rid of one indentation level?
if not parts:
break
if parts[0] == "/":
...There was a problem hiding this comment.
Maybe, I'll try.
Wrote the method by looking on generated output and tuning code.
Perhaps it can be rewritten for less repetition
| if len(parts) > 1: | ||
| if parts[0] == "/": | ||
| if len(parts) < 3: | ||
| slash = "/" |
There was a problem hiding this comment.
slash is not used after the loop.
There was a problem hiding this comment.
It is, pre = f"//{url.host or ''}{slash}"
| slash = "" | ||
| # len(parts) > 1 always | ||
| first, second, *last = parts | ||
| if first == "...": |
There was a problem hiding this comment.
Seems the part after if first == "...": is common in both branches.
| else: | ||
| # there is only top folder line, drop next file line | ||
| del self.lines[1] | ||
| for lineno, line in enumerate(self.lines): |
There was a problem hiding this comment.
If there was a problem with 1-based linenos, you could use enumerate(self.lines, 1).
There was a problem hiding this comment.
I know about enumerate(it, start) variant.
The reason for conversion to 0-based index is mainly the fact that list and other Python types are 0-based too. There is no motivation for 1-based ones, Python is not Pascal.
The code can be rewritten with 1-based indexes, sure. I just think that 0-based is more readable.
