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

check-ignore: exit --stdin silently on empty string [qa] #4361

Closed
jorgeorpinel opened this issue Aug 8, 2020 · 6 comments · Fixed by #4368
Closed

check-ignore: exit --stdin silently on empty string [qa] #4361

jorgeorpinel opened this issue Aug 8, 2020 · 6 comments · Fixed by #4368
Labels
enhancement Enhances DVC ui user interface / interaction

Comments

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Aug 8, 2020

Extracted from #4323 (comment)

When you use check-ignore in interactive mode, empty strings produce an error and exit. However empty strings are accepted (never match) without --stdin:

λ cat .dvcignore
*

λ dvc check-ignore --stdin
 anything
anything
 # <- just hit Enter
Empty string is not a valid pathspec. Please use . instead if you meant to match all paths.

λ dvc check-ignore -dn ''
::

So I propose that a) empty strings in --stdin interactive mode simply cause the tool to exit back to the shell prompt and/or b) empty strings sent as targets should just be ignored? (exit tool silently)

@jorgeorpinel jorgeorpinel added enhancement Enhances DVC ui user interface / interaction labels Aug 8, 2020
@jorgeorpinel jorgeorpinel changed the title check-ignore: exit --stdin silently on empty string check-ignore: exit --stdin silently on empty string [qa] Aug 8, 2020
@jorgeorpinel
Copy link
Contributor Author

Cc @karajan1001 WDYT?

@jorgeorpinel
Copy link
Contributor Author

And as an extra (from #4323 (review)), the cannot use both `--details` and `--quiet` error could also be avoided by just having --quiet win i.e. --details could imply have no effect if combined with --quiet.

@karajan1001
Copy link
Contributor

karajan1001 commented Aug 9, 2020

@jorgeorpinel
For empty string error. I just followed what the Git do,
check-ignore-error
And they didn't mention this in there documents.

So the question is do we need another exiting method besides Ctrl+C. If so, a) plan and mention it in our doc, if not just b) plan.


"cannot use both --details and --quiet" I agree.

@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Aug 9, 2020

I see. Well, Git has notoriously poor interface and docs, so not a role model on every single detail 🙂

BTW, git check-ignore --stdin doesn't throw an error on Ctrl+C.

do we need another exiting method besides Ctrl+C

I think my suggestion to use Enter makes for an intuitive UI (except if you're used to git check-ignore but I doubt it's a well known command). And yes either way we'll have a small mention of how to exit --stdin interactive mode in docs.

Thanks!

@karajan1001
Copy link
Contributor

karajan1001 commented Aug 10, 2020

I'm sorry, I'm not quite understanding here. Only Enter without other things gives an empty string to the program if we use input() this is what we are doing here.

karajan1001 added a commit to karajan1001/dvc that referenced this issue Aug 10, 2020
fix iterative#4361
1. Remove error message when input a empty string
2. Remove error message when read input from stream
2. Remove error message when interrupt by `CTRL-C`
efiop pushed a commit that referenced this issue Aug 10, 2020
fix #4361
1. Remove error message when input a empty string
2. Remove error message when read input from stream
2. Remove error message when interrupt by `CTRL-C`
@jorgeorpinel
Copy link
Contributor Author

Only Enter without other things gives an empty string to the program

Ohhh good point sorry, I was confused about that. My idea was to end the interactive mode on empty string then. I see you've already done that in #4368, thanks @karajan1001 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhances DVC ui user interface / interaction
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants