Add warning if nothing was searched

[goto-engineering]

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]

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.

[BurntSushi]

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]

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]

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]

@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]

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