Skip to content

Subcommand help #3956

Merged
efiop merged 14 commits into
treeverse:masterfrom
aerubanov:subcommand_help
Jun 5, 2020
Merged

Subcommand help #3956
efiop merged 14 commits into
treeverse:masterfrom
aerubanov:subcommand_help

Conversation

@aerubanov
Copy link
Copy Markdown
Contributor

@aerubanov aerubanov commented Jun 4, 2020

Fixes #2395

  • ❗ I have followed the Contributing to DVC checklist.

  • 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.

  • ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Comment thread tests/func/test_cli.py Outdated
Comment on lines +178 to +192
class TestUnknownCommandHelp(TestDvc):
def test(self):
with io.StringIO() as buf, redirect_stdout(buf):
try:
_ = parse_args(["unknown"])
except DvcParserError:
pass
output = buf.getvalue()
with io.StringIO() as buf, redirect_stdout(buf):
try:
_ = parse_args(["--help"])
except SystemExit:
pass
help_out = buf.getvalue()
self.assertEqual(output, help_out)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you please convert these tests to pytest-style? We are moving away from unittest to pytest these days, and all new tests are prefered to be in pytest. Sorry for the confusion :(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@efiop, Should I use dvc fixture from tests/dir_helpers instead of TestDvc class? Or something else?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@aerubanov Yes, that's the one. But actually, you might not need any dvc fixtures at all. Right now I'm not seeing anything that needs dvc-repo. E.g. see tests/unit/command/test_get.py.

Comment thread tests/func/test_cli.py Outdated
except DvcParserError:
pass
output = buf.getvalue()
with io.StringIO() as buf, redirect_stdout(buf):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should be able to use capsys in pytest-style test.

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.

Thank you so much @aerubanov ! Please see a few comments above.

pmrowla and others added 3 commits June 4, 2020 23:22
* remote: move upload()/download() into tree

* remote: move path_info/path_cls into tree

* remote.azure: finish moving methods into tree

* remote.gs: finish moving methods into tree

* remote.hdfs: finish moving methods to tree

* remote.http: finish moving methods into tree

* remote.oss: finish moving methods into tree

* remote.s3: finish moving methods into tree

* remote.ssh: finish moving methods into tree

* remote.gdrive: finish moving methods into tree

* tests: update remote unit tests

* tests: update func tests

* remote: use walk_files() for all remotes

* list_cache_paths() now uses tree.walk_files() for all remotes except
  local/ssh

* fix DS warnings

* bugfixes
* run: disable directories for -m/-M flags

* run: reformatted string literals

* run: simplified metrics check

* tests: func: run: dirs as metrics outs

* tests: func: run: reformatted string literals

* tests: use pytest

Co-authored-by: Ruslan Kuprieiev <ruslan@iterative.ai>
* refactor multistage load for params and outputs

* tests: load params

* tests: output loading from pipeline file

* fix test

* fix typo in name

* split params load

* rename params func s/inject_values/fill_values

* fix tests

* simplify loads_params and output.load_from_pipeline

* address @pared's suggestions for tests
Comment thread dvc/cli.py Outdated
@aerubanov
Copy link
Copy Markdown
Contributor Author

I changed the tests to pytest-style and removed the getttext.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 5, 2020

Codecov Report

Merging #3956 into master will increase coverage by 0.08%.
The diff coverage is 93.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3956      +/-   ##
==========================================
+ Coverage   92.15%   92.24%   +0.08%     
==========================================
  Files         161      161              
  Lines       11180    11160      -20     
==========================================
- Hits        10303    10294       -9     
+ Misses        877      866      -11     
Impacted Files Coverage Δ
dvc/command/run.py 89.79% <ø> (ø)
dvc/remote/oss.py 93.75% <84.61%> (-0.54%) ⬇️
dvc/remote/azure.py 93.75% <87.87%> (-1.60%) ⬇️
dvc/remote/ssh/__init__.py 94.38% <88.88%> (-0.09%) ⬇️
dvc/remote/gdrive.py 81.18% <90.47%> (+0.28%) ⬆️
dvc/remote/s3.py 94.83% <92.95%> (-0.38%) ⬇️
dvc/remote/http.py 94.28% <93.47%> (+0.11%) ⬆️
dvc/remote/hdfs.py 93.33% <95.23%> (+0.41%) ⬆️
dvc/remote/base.py 95.71% <95.45%> (+0.06%) ⬆️
dvc/remote/gs.py 97.39% <96.96%> (-0.23%) ⬇️
... and 10 more

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 72a576c...f5ddbec. Read the comment docs.

@efiop efiop merged commit 2c9cb87 into treeverse:master Jun 5, 2020
@efiop
Copy link
Copy Markdown
Contributor

efiop commented Jun 5, 2020

Thank you @aerubanov !

@aerubanov aerubanov deleted the subcommand_help branch June 6, 2020 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Help for subcommand

6 participants