dvc: "DVC-file" term revision, `targets` argument help update, other small changes #2112

jorgeorpinel · 2019-06-10T23:12:55Z

Also fixes #2169

Have you followed the guidelines in our
Contributing document?
Does your PR affect documented changes or does it add new functionality
that should be documented? See PR standard way to refer to DVC-files dvc.org#422

efiop · 2019-06-11T06:50:45Z

@jorgeorpinel Looks good! 🙂 Just a heads up: tests failed, since you need to modify them to when you are modifying logger output.

jorgeorpinel · 2019-06-22T05:27:15Z

Thanks @efiop and sorry for the large delay in fixing this. I was aware that tests have to be changed but in this case I just had an extra letter 's' in the string accidentally 💀

p.s. I'll add the fix for #2169 in this PR unless you get to merging it first.

jorgeorpinel · 2019-06-23T04:54:19Z

Quick note: I just added a small fix to the Zsh autocomplete script in e3ab064, unrelated to anything else mentioned here so far.

ghost

lgtm 👍

dvc/repo/pkg/install.py

dvc/stage.py

tests/func/test_add.py

shcheklein

A few comments to address. Thanks for updating the commands help.

Keep "stage file" in the context of pipelines No issue

To close #2112

Fix #2169

For #2112

and matching jorgeorpinel/dvc.org@dd13134

jorgeorpinel · 2019-06-27T04:31:28Z

Please check my last 2 commits (91bd0fa...936f941) which introduce new changes to this PR. Thanks

data sync and `commit` commands, since `None` is the default value for the removed `default` property, see https://docs.python.org/dev/library/argparse.html#default

jorgeorpinel · 2019-06-27T05:34:05Z

dvc/command/commit.py

@@ -65,6 +65,9 @@ def add_parser(subparsers, parent_parser):
        help="Commit cache for subdirectories of the specified directory.",
    )
    commit_parser.add_argument(
-        "targets", nargs="*", default=None, help="DVC-files."


Please also notice that I removed the default property from here and from the same arg (targets) in data_sync.py, because None is the default value for default anyway (and no other call to add_argument("targets", ...) in the repo sends it).

However, argparse code for cache dir/value, config/value, data sync/-j, diff/-t and b_ref, gc/-j and -p, and run/-c and -w still explicitly send an unnecessary default=None (unless I'm wrong and it's not unnecessary). Should I remove it from all those as well? Thanks

On this, I'll take the liberty to remove the default=Nones from add_argument calls for a future PR. Thanks

dvc/command/lock.py

dvc/command/remove.py

scripts/completion/dvc.zsh

efiop · 2019-06-27T08:47:01Z

tests/func/test_add.py

@@ -475,6 +475,7 @@ def test(self):
        self.assertEqual(0, ret)

        foo_stage_file = self.FOO + Stage.STAGE_FILE_SUFFIX
+


I had changed the comment bellow from "stage file" from "DVC-file" but Ivan said to leave comments alone so I reverted it. I also added that new line to match the other similar code block in class TestShouldThrowProperExceptionOnCorruptedStageFile (same file).

I was refering to the newline itself. It is polluting the git blame 🙂 Let's leave it this time, but please don't leave such things in the future.

dvc/stage.py

Per #2112 (review)

Continuation of fix in 5ba3b54

efiop

Thank you!

jorgeorpinel · 2019-06-27T20:28:45Z

Thanks!

This comment has been minimized.

Sign in to view

jorgeorpinel changed the title ~~dvc: Use "DVC-file" term as much as possible (vs. "stage file")~~ [WIP] dvc: Use "DVC-file" term as much as possible (vs. "stage file") Jun 11, 2019

This comment has been minimized.

Sign in to view

jorgeorpinel mentioned this pull request Jun 12, 2019

there's ambiguity between the terms "stage" and "stage file" iterative/dvc.org#423

Closed

efiop added the refactoring Factoring and re-factoring label Jun 18, 2019

efiop assigned jorgeorpinel Jun 18, 2019

jorgeorpinel changed the title ~~[WIP] dvc: Use "DVC-file" term as much as possible (vs. "stage file")~~ dvc: Use "DVC-file" term as much as possible (vs. "stage file") Jun 22, 2019

jorgeorpinel mentioned this pull request Jun 25, 2019

usage: several problems in output text #2169

Closed

ghost approved these changes Jun 25, 2019

View reviewed changes

efiop requested changes Jun 25, 2019

View reviewed changes

dvc/repo/pkg/install.py Outdated Show resolved Hide resolved

shcheklein reviewed Jun 25, 2019

View reviewed changes

dvc/stage.py Outdated Show resolved Hide resolved

shcheklein reviewed Jun 25, 2019

View reviewed changes

tests/func/test_add.py Outdated Show resolved Hide resolved

shcheklein requested changes Jun 25, 2019

View reviewed changes

jorgeorpinel added 6 commits June 26, 2019 14:05

dvc: Use "DVC-file" term as much as possible

fba7d85

Keep "stage file" in the context of pipelines No issue

tests: fix failing test_add (due to extra 's' in word)

f004a84

To close #2112

scripts: fix zsh autocomplete for dvc pipeline subcommands

5b2405f

help: fixed -h output for diff and remote subcommands

79b22fc

Fix #2169

pkg: rm dvc/repo/pkg/install.py

bac1e60

For #2112

dvc: feedback to #2112 round 1

91bd0fa

shcheklein approved these changes Jun 26, 2019

View reviewed changes

jorgeorpinel added 2 commits June 26, 2019 21:07

data sync: small improvement continuing #2121

e3f1e61

and matching jorgeorpinel/dvc.org@dd13134

help: update usage for commands that use the targets argument

936f941

jorgeorpinel changed the title ~~dvc: Use "DVC-file" term as much as possible (vs. "stage file")~~ dvc: "DVC-file" term revision, target argument help update, other small changes Jun 27, 2019

command: remove default property from "targets" argparse code in

a927a50

data sync and `commit` commands, since `None` is the default value for the removed `default` property, see https://docs.python.org/dev/library/argparse.html#default

jorgeorpinel commented Jun 27, 2019

View reviewed changes

efiop requested changes Jun 27, 2019

View reviewed changes

dvc/command/lock.py Outdated Show resolved Hide resolved

efiop mentioned this pull request Jun 27, 2019

lock, unlock: nothing happens without targets arg #2199

Closed

efiop reviewed Jun 27, 2019

View reviewed changes

dvc/command/lock.py Outdated Show resolved Hide resolved

efiop reviewed Jun 27, 2019

View reviewed changes

dvc/command/remove.py Outdated Show resolved Hide resolved

efiop reviewed Jun 27, 2019

View reviewed changes

scripts/completion/dvc.zsh Show resolved Hide resolved

efiop reviewed Jun 27, 2019

View reviewed changes

dvc/stage.py Outdated Show resolved Hide resolved

jorgeorpinel added 2 commits June 27, 2019 12:23

command: rever accidental narg="+" changes in recent commit

5ba3b54

Per #2112 (review)

stage: fix typo "an"->"a"

d0718cd

Per #2112 (review)

jorgeorpinel changed the title ~~dvc: "DVC-file" term revision, target argument help update, other small changes~~ dvc: "DVC-file" term revision, targets argument help update, other small changes Jun 27, 2019

jorgeorpinel added 2 commits June 27, 2019 13:39

command: add missing space in optional targets ard usage output

a2f3d7f

lock: lock/unlock targets are NOT optional.

5bf6eaa

Continuation of fix in 5ba3b54

efiop approved these changes Jun 27, 2019

View reviewed changes

efiop merged commit 8c2fa8d into iterative:master Jun 27, 2019

jorgeorpinel mentioned this pull request Jun 27, 2019

import: add import and get to autocomplete scripts; et al #2205

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dvc: "DVC-file" term revision, `targets` argument help update, other small changes #2112

dvc: "DVC-file" term revision, `targets` argument help update, other small changes #2112

jorgeorpinel commented Jun 10, 2019 •

edited

This comment has been minimized.

This comment has been minimized.

efiop commented Jun 11, 2019

jorgeorpinel commented Jun 22, 2019

jorgeorpinel commented Jun 23, 2019 •

edited

ghost left a comment

shcheklein left a comment

jorgeorpinel commented Jun 27, 2019

jorgeorpinel Jun 27, 2019 •

edited

jorgeorpinel Jun 27, 2019 •

edited

efiop Jun 27, 2019

jorgeorpinel Jun 27, 2019 •

edited

efiop Jun 27, 2019

efiop left a comment

jorgeorpinel commented Jun 27, 2019

		@@ -475,6 +475,7 @@ def test(self):
		self.assertEqual(0, ret)

		foo_stage_file = self.FOO + Stage.STAGE_FILE_SUFFIX

dvc: "DVC-file" term revision, targets argument help update, other small changes #2112

dvc: "DVC-file" term revision, targets argument help update, other small changes #2112

Conversation

jorgeorpinel commented Jun 10, 2019 • edited

This comment has been minimized.

This comment has been minimized.

efiop commented Jun 11, 2019

jorgeorpinel commented Jun 22, 2019

jorgeorpinel commented Jun 23, 2019 • edited

ghost left a comment

Choose a reason for hiding this comment

shcheklein left a comment

Choose a reason for hiding this comment

jorgeorpinel commented Jun 27, 2019

jorgeorpinel Jun 27, 2019 • edited

Choose a reason for hiding this comment

jorgeorpinel Jun 27, 2019 • edited

Choose a reason for hiding this comment

efiop Jun 27, 2019

Choose a reason for hiding this comment

jorgeorpinel Jun 27, 2019 • edited

Choose a reason for hiding this comment

efiop Jun 27, 2019

Choose a reason for hiding this comment

efiop left a comment

Choose a reason for hiding this comment

jorgeorpinel commented Jun 27, 2019

dvc: "DVC-file" term revision, `targets` argument help update, other small changes #2112

dvc: "DVC-file" term revision, `targets` argument help update, other small changes #2112

jorgeorpinel commented Jun 10, 2019 •

edited

jorgeorpinel commented Jun 23, 2019 •

edited

jorgeorpinel Jun 27, 2019 •

edited

jorgeorpinel Jun 27, 2019 •

edited

jorgeorpinel Jun 27, 2019 •

edited