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

Add warning if nothing was searched #1762

Conversation

goto-engineering
Copy link

Ok, another attempt at fixing #1404.

I somehow messed up the other PR in the Github UI, not sure how. You can delete it.

Let me know if this is on the right track, and I'll look into tests.

@BurntSushi
Copy link
Owner

I somehow messed up the other PR in the Github UI, not sure how. You can delete it.

Please don't delete PRs or issues. I'm really surprised GitHub gives you the ability to delete issues/PRs filed on another repo. Please don't do that. Because you deleted it, we've now lost some historical context and some helpful info on a false start. Sure, the PR might have been wrong, but we don't just sweep wrong things under the rug. We learn from them.

Aside from that, you shouldn't have needed to open a new PR. For a small change like this, it's enough to keep it inside a single commit. When you make a change, just amend the commit. (git commit -a --amend for example.) And since the history is altered, you'll need to force push it, e.g., git push <remote> <branch> --force.

Copy link
Owner

@BurntSushi BurntSushi 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 this looks correct to me! Have you tried it? Does it work?

I think adding some tests would be a good idea. They should go in tests/feature.rs. I think you'll find assert_non_empty_stderr() helpful in this case. Here's an example of another test that uses it:

ripgrep/tests/feature.rs

Lines 44 to 47 in a6d0547

// See: https://github.com/BurntSushi/ripgrep/issues/1
rgtest!(f1_unknown_encoding, |_: Dir, mut cmd: TestCommand| {
cmd.arg("-Efoobar").assert_non_empty_stderr();
});

Take a look at other tests in that file to see how to setup directory trees and what not.

@goto-engineering
Copy link
Author

I somehow messed up the other PR in the Github UI, not sure how. You can delete it.

Please don't delete PRs or issues. I'm really surprised GitHub gives you the ability to delete issues/PRs filed on another repo. Please don't do that. Because you deleted it, we've now lost some historical context and some helpful info on a false start. Sure, the PR might have been wrong, but we don't just sweep wrong things under the rug. We learn from them.

Aside from that, you shouldn't have needed to open a new PR. For a small change like this, it's enough to keep it inside a single commit. When you make a change, just amend the commit. (git commit -a --amend for example.) And since the history is altered, you'll need to force push it, e.g., git push <remote> <branch> --force.

Sorry, I got confused with the whole fork thing on github. Since there was only 1 commit, I deleted the branch on my fork, not realizing that would close the PR. I had intended to just force push my new commit to the same branch name.

@goto-engineering
Copy link
Author

I think this looks correct to me! Have you tried it? Does it work?

I think adding some tests would be a good idea. They should go in tests/feature.rs. I think you'll find assert_non_empty_stderr() helpful in this case. Here's an example of another test that uses it:

ripgrep/tests/feature.rs

Lines 44 to 47 in a6d0547

// See: https://github.com/BurntSushi/ripgrep/issues/1
rgtest!(f1_unknown_encoding, |_: Dir, mut cmd: TestCommand| {
cmd.arg("-Efoobar").assert_non_empty_stderr();
});

Take a look at other tests in that file to see how to setup directory trees and what not.

Yes, I've tried it with this setup of files:

.ignore - (ignored-dir/**)
ignored-dir/file.txt ("needle")
other-file.txt (doesn't contain needle)

Then I do rg needle. If other-file.txt is there, it doesn't show the warning. If only ignored-dir is there, it does show up.

Thanks for the hints, I'll look into the tests later!

@goto-engineering
Copy link
Author

goto-engineering commented Dec 18, 2020

@BurntSushi I added a test case for the warning and it passes. But another test (regression::r428_unrecognized_style) fails because its setup also triggers the warning.

I think it's because the case being tested there throws an exception and therefore never triggers my searched = true. I'm not really sure how to handle that. Since it's about an unrecognized style, is this happening in the builder itself?

@goto-engineering
Copy link
Author

@BurntSushi Ping just in case you hadn't seen this

@BurntSushi BurntSushi added the rollup A PR that has been merged with many others in a rollup. label May 30, 2021
BurntSushi pushed a commit that referenced this pull request May 30, 2021
This was once part of ripgrep, but at some point, was unintentionally
removed. The value of this warning is that since ripgrep tries to be
"smart" by default, it can be surprising if it doesn't search certain
things. This warning covers the case when ripgrep searches *nothing*,
which happens somewhat more frequently than you might expect. e.g., If
you're searching within an ignore directory.

Fixes #1404, Closes #1762
BurntSushi pushed a commit that referenced this pull request May 31, 2021
This was once part of ripgrep, but at some point, was unintentionally
removed. The value of this warning is that since ripgrep tries to be
"smart" by default, it can be surprising if it doesn't search certain
things. This warning covers the case when ripgrep searches *nothing*,
which happens somewhat more frequently than you might expect. e.g., If
you're searching within an ignore directory.

Note that for now, we only print this message when the user has not
supplied any explicit paths. It's not clear that we want to print this
otherwise, and in particular, it seems that the message shows up too
eagerly. e.g., 'rg foo does-not-exist' will both print an error about
'does-not-exist' not existing, *and* the message about no files being
searched, which seems annoying in this case. We can always refine this
logic later.

Fixes #1404, Closes #1762
BurntSushi pushed a commit that referenced this pull request Jun 1, 2021
This was once part of ripgrep, but at some point, was unintentionally
removed. The value of this warning is that since ripgrep tries to be
"smart" by default, it can be surprising if it doesn't search certain
things. This warning covers the case when ripgrep searches *nothing*,
which happens somewhat more frequently than you might expect. e.g., If
you're searching within an ignore directory.

Note that for now, we only print this message when the user has not
supplied any explicit paths. It's not clear that we want to print this
otherwise, and in particular, it seems that the message shows up too
eagerly. e.g., 'rg foo does-not-exist' will both print an error about
'does-not-exist' not existing, *and* the message about no files being
searched, which seems annoying in this case. We can always refine this
logic later.

Fixes #1404, Closes #1762
@BurntSushi BurntSushi closed this in e6cac8b Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rollup A PR that has been merged with many others in a rollup.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants