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

Implement formatters for ls operation #469

Merged
merged 28 commits into from
Feb 8, 2019
Merged

Implement formatters for ls operation #469

merged 28 commits into from
Feb 8, 2019

Conversation

shagren
Copy link
Contributor

@shagren shagren commented Feb 4, 2019

Not implemented:

  • Tests


click.echo(StorageLsFormatter()(res))
for line in layout.format(formatter, files):
Copy link
Contributor Author

@shagren shagren Feb 4, 2019

Choose a reason for hiding this comment

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

it's generator. We can use click.pager() here instead click.echo()

@shagren shagren changed the title [WIP] Implement simple formatter for ls operation [WIP] Implement formatters for ls operation Feb 5, 2019
python/Makefile Outdated
@@ -10,7 +10,7 @@ ISORT_REGEXP := ^python/(neuromation|tests|build-tools)/.+\\.py
BLACK_DIRS := $(ISORT_DIRS)
BLACK_REGEXP := $(ISORT_REGEXP)
MYPY_DIRS := neuromation
MYPY_REGEXP := ^python/+.\\py
MYPY_REGEXP := ^python/neuromation/+.\\py
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Why?
  2. The change is not related to the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. It`s right. REGEXP's variables works from project root but DIR's variables from python folder.
  2. Yes. Can move to separate mini-pr

@@ -48,21 +61,232 @@ def storage() -> None:

@command()
@click.argument("path", default="storage://~")
@click.option(
"-C", "force_format_vertical", is_flag=True, help="list entries by columns"
Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask why did you add all these options?
Where is the corresponding discussion?
#427 mentions only -l and (indirectly) -h.

Copy link
Contributor Author

@shagren shagren Feb 6, 2019

Choose a reason for hiding this comment

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

I investigated how work gnu ls and find that it provide 3 formats/layouts according scenarios from #427:

  • long, detailed file info, one file per line
  • vertical, file names which displayed in few columns and sorted from top-down, left-right. It's deault layout for terminal.
  • single-column, file names, one per line. This layout will be enabled automatically when stdout is not terminal and layout is not specified.

Then i learned that we need some more that "-l" switch and add --format option with one additional value commas and next shortcuts: -l, -x, -1, -C, .
After that I consider that commas format often require quoting for file names and add --quote-name(and alias -Q) and inveredt(but default) options --literal(with alias -N)

Also i found that ls sort files by alphabet by default and add --sort option with shortcuts(-f, -S, -t, -U, -r / --reverse).

We haven`t before any time field in ls output but we have modification_time field in FileStatus. As I remember this fields was added some later than neuro storage ls realized, and than this information was missed. I add this information to long format and corresponding options --time-style, --full-time.

There left only one option which not follow initial discussion: --group-directories-first.

That's all.

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

I think the PR cannot land as is: rare options are not only useless but actually harmful.
They blow documentation size and make learning the tool harder without a strong reason.

Also, it makes the support of existing code harder than needed

@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

Merging #469 into master will increase coverage by 0.3%.
The diff coverage is 96.8%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #469     +/-   ##
=========================================
+ Coverage   90.61%   90.91%   +0.3%     
=========================================
  Files          36       37      +1     
  Lines        2439     2553    +114     
  Branches      282      313     +31     
=========================================
+ Hits         2210     2321    +111     
- Misses        178      179      +1     
- Partials       51       53      +2
Impacted Files Coverage Δ
python/neuromation/cli/rc.py 93.96% <100%> (+0.06%) ⬆️
python/neuromation/cli/formatter.py 100% <100%> (ø) ⬆️
python/neuromation/cli/main.py 56.06% <80%> (+1.03%) ⬆️
python/neuromation/cli/storage.py 96.03% <85.71%> (-1.77%) ⬇️
python/neuromation/cli/files_formatter.py 99.01% <99.01%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3705c2...a6ec4a4. Read the comment docs.

@shagren shagren changed the title [WIP] Implement formatters for ls operation Implement formatters for ls operation Feb 7, 2019
@shagren
Copy link
Contributor Author

shagren commented Feb 7, 2019

@asvetlov , please check now

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Looks good!
A few comments

python/neuromation/cli/files_formatter.py Outdated Show resolved Hide resolved
python/neuromation/cli/files_formatter.py Outdated Show resolved Hide resolved
python/neuromation/cli/files_formatter.py Outdated Show resolved Hide resolved
python/neuromation/cli/files_formatter.py Outdated Show resolved Hide resolved
python/neuromation/cli/files_formatter.py Outdated Show resolved Hide resolved
python/neuromation/cli/storage.py Outdated Show resolved Hide resolved
python/neuromation/cli/files_formatter.py Outdated Show resolved Hide resolved
python/setup.cfg Show resolved Hide resolved
python/tests/cli/test_formatter.py Outdated Show resolved Hide resolved
python/neuromation/cli/storage.py Outdated Show resolved Hide resolved
Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants