Two new modes `--all` and `--stdin` for check-ignore by karajan1001 · Pull Request #4323 · treeverse/dvc

karajan1001 · 2020-08-03T11:45:14Z

Fix #4321

Introduce new arguments --all for dvc check-ignore
Add tests for dvc check-ignore --all

❗ 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.

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

Fix treeverse#4321 Introduce new arguments --all for dvc check-ignore Add tests for dvc check-ignore --all

pared

LGTM, one change request.

1. `--stdin` and targets can and only can have one 2. more tests

karajan1001 · 2020-08-04T14:58:19Z

LGTM, one change request.
@pared
Thank you, I'm so careless to write such a bug.

And I had a problem. I tried to add a pytest to --stdin mode but failed.

I used monkeypatch to mock user input but was blocked by
https://github.com/iterative/dvc/blob/35dd1fda9cfb4b645ae431f4621f2d1853d4fb7c/dvc/prompt.py#L11
Are there any methods to pass this line?

pared · 2020-08-04T15:18:46Z

@karajan1001
I guess most straightforward would be to mock isatty too, though I will see if there is something else to be done here.

codecov · 2020-08-04T21:15:21Z

Codecov Report

Merging #4323 into master will decrease coverage by 0.04%.
The diff coverage is 71.05%.

@@            Coverage Diff             @@
##           master    #4323      +/-   ##
==========================================
- Coverage   91.24%   91.19%   -0.05%     
==========================================
  Files         177      177              
  Lines       12182    12209      +27     
==========================================
+ Hits        11115    11134      +19     
- Misses       1067     1075       +8

Impacted Files	Coverage Δ
dvc/command/experiments.py	`45.12% <0.00%> (-0.34%)`	⬇️
dvc/repo/experiments/__init__.py	`85.71% <72.22%> (-1.79%)`	⬇️
dvc/updater.py	`86.58% <100.00%> (ø)`

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 11d906e...a504fa3. Read the comment docs.

1. Add a new tests with mock stdin for interactive mode 2. Solve the problem of `assert "" in caplog.text` in tests always `True`.

karajan1001 · 2020-08-05T01:54:47Z

@karajan1001
I guess most straightforward would be to mock isatty too, though I will see if there is something else to be done here.

@pared
Oh thank you very much, it works. But I don't know if there are some risks to influence other tests.

pared · 2020-08-05T12:17:29Z

@karajan1001

But I don't know if there are some risks to influence other tests.

When you are using a mocker inside a test, patches and mocks are applied only to this test. So, if you are worried, whether it's possible to "leak" the mock to other tests - no, it should not happen.

I will see if there is something else to be done here

I think it's fine to use this patch. isatty tests whether we are working from terminal, so it is a situation we are trying to simulate.

jorgeorpinel · 2020-08-06T04:34:06Z

Hi!

Maybe you can address #4282 (review) and #4282 (review) here @karajan1001 ? Thanks!

efiop · 2020-08-06T06:25:59Z

Maybe you can address #4282 (review) and #4282 (review) here @karajan1001 ? Thanks!

@jorgeorpinel Let's not do that. Let's not expand the scope of this PR, those changes are completely unrelated.

karajan1001 · 2020-08-06T06:30:03Z

Maybe you can address #4282 (review) and #4282 (review) here @karajan1001 ? Thanks!

@jorgeorpinel Let's not do that. Let's not expand the scope of this PR, those changes are completely unrelated.

I had considered doing so, but give up. BTW, doc of this PR needs to be reviewed as well.

1. remove path_input and make _ask to a public method instead

efiop · 2020-08-06T19:31:09Z

Thank you @karajan1001 ! 🙏

There are some tiny inconsistencies with stuff like messages, but we'll adjust them along the way. It is great to have such a powerful tool in our toolbox!

jorgeorpinel · 2020-08-07T02:23:05Z

BTW, doc of this PR needs to be reviewed as well.

@karajan1001 is there a docs PR for this already? We haven't even merged the previous one 😅 (see treeverse/dvc.org#1629 (comment))

karajan1001 · 2020-08-07T02:43:07Z

@jorgeorpinel

Not already, I'm waiting for this one. And it would be created soon.

jorgeorpinel · 2020-08-07T20:50:07Z

+                    "Empty string is not a valid pathspec. Please use . "
+                    "instead if you meant to match all paths."


What is a pathspec?

Can users actually send an empty string (e.g. '')? Or does this happen when you don't send any args?

@jorgeorpinel

pathspec means the target to be checked

It is in an interactive mode where users can type string as the target, and this happened when users type send an empty string.

It is in an interactive mode

So maybe don't say anything and just go to the next > ? Seems intuitive enough that way. I just tried it and noticed it crashes after this error msg (i.e. it stops the interactive mode).

UPDATE: On Windows at least, there's no > chars in interactive mode, see treeverse/dvc.org#1675 (review)

p.s. when I send an empty string as target i.e. dvc check-ignore '' it just checks it (never matches even if * is a pattern in .dvcignore) and this error isn't presented. Seems slightly inconsistent. I vote to just remove it altogether 🙂

Extracted to #4361

jorgeorpinel · 2020-08-07T20:51:18Z

+        if self.args.quiet and self.args.details:
+            raise DvcException("cannot use both `--details` and `--quiet`")


Maybe --quiet should just win here?

Maybe --quiet should just win here?

Yes, it used to be --verbose and --quiet. They might cause some problems, and had been fixed in #4304.
And for --details and --quiet we can just let --quiet win.

jorgeorpinel

Left some questions @karajan1001 ☝️

We can discuss my other suggestions in treeverse/dvc.org/pull/1675 and I'll apply them (probably in #4358).

per treeverse/dvc#4323 (review)

per treeverse/dvc#4323 (comment)

per #4323 et al.

* cmd: remove unnecessary commas in get and import * cmd: fix typo in add * cmd: remote copy edits per #1617 (comment) * guide: .dvcignore copy edit * cmd: init copy edits * clarify about dirs in import -o * cmd: review get -o desc * dvcignore: updates to guide and check-ignore ref. per #1629 (review) et al. * cmd: update check-ignore -n per treeverse/dvc#4323 (comment) * cmd: fix get.import -o descriptions per #1673 (review) and #1673 (review) * cmd: copy edits to remote add/modify

* check_ignore: update help output per #4282 (review) and #4282 (review) * check-ignore: more output string updates per #4323 et al. * check-ignore: min text update to match treeverse/dvc.org/pull/1673 Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>

Two new mode -all and --stdin for check-ignore

aa27920

Fix treeverse#4321 Introduce new arguments --all for dvc check-ignore Add tests for dvc check-ignore --all

efiop requested a review from pared August 3, 2020 11:52

efiop changed the title ~~Two new mode -all and --stdin for check-ignore~~ Two new modes --all and --stdin for check-ignore Aug 4, 2020

karajan1001 mentioned this pull request Aug 4, 2020

Docs for new command dvc check-ignore treeverse/dvc.org#1629

Merged

pared suggested changes Aug 4, 2020

View reviewed changes

Comment thread dvc/command/check_ignore.py Outdated

--stdin and targets only one

fc40747

1. `--stdin` and targets can and only can have one 2. more tests

New tests for stdin mode and solve a problem

489dd16

1. Add a new tests with mock stdin for interactive mode 2. Solve the problem of `assert "" in caplog.text` in tests always `True`.

pared approved these changes Aug 5, 2020

View reviewed changes