Fix reiteration of the first found match with --only-mathing flag #454

kpp · 2017-04-20T15:05:38Z

Fixes #451

~/ripgrep$ cat tests/digits.txt 
1 2 3
123
~/ripgrep$ cargo run -- "\d" tests/digits.txt -o --column
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running `target/debug/rg '\d' tests/digits.txt -o --column`
1:1:1
1:3:2
1:5:3
2:1:1
2:2:2
2:3:3
~/ripgrep$ cargo run -- "\d+" tests/digits.txt -o --column
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running `target/debug/rg '\d+' tests/digits.txt -o --column`
1:1:1
1:3:2
1:5:3
2:1:123

BurntSushi

Thanks for this fix! I think this mostly looks good, but I left some nits on the test. I also think we should include a regression test that roughly matches what the reporter initially presented.

BurntSushi · 2017-04-20T15:07:25Z

tests/tests.rs

+    wd.create(path, "1 2 3\n123\n");
+
+    let mut cmd = wd.command();
+    cmd.arg("\\d").arg(path)


Could you use a raw string literal for this? e.g., r"\d".

BurntSushi · 2017-04-20T15:07:38Z

tests/tests.rs

+    let mut cmd = wd.command();
+    cmd.arg("\\d").arg(path)
+        .arg("--only-matching")
+        .arg("--color=never")


I don't think you need this.

You are right

BurntSushi · 2017-04-20T15:08:22Z

tests/tests.rs

@@ -1607,6 +1607,32 @@ fn regression_391() {
    assert_eq!(lines, "bar.py\n");
 }

+// See: https://github.com/BurntSushi/ripgrep/issues/451
+#[test]
+fn regression_451_only_matching() {


This test looks good. Could you add another test that approximately mimics the bug report in #451? e.g., Test that the actual results printed are correct instead of just column numbers.

Ah. I see. Yep

Fixes BurntSushi#451

kpp · 2017-04-21T11:42:26Z

@BurntSushi ?

BurntSushi · 2017-04-21T12:12:02Z

Looks good, thank you!

BurntSushi requested changes Apr 20, 2017

View reviewed changes

kpp force-pushed the bug_451_only_mathing_digits branch 2 times, most recently from 8e83224 to 8539784 Compare April 20, 2017 15:32

Fix reiteration of the first found match with --only-mathing flag

5854b69

Fixes BurntSushi#451

kpp force-pushed the bug_451_only_mathing_digits branch from 8539784 to 5854b69 Compare April 20, 2017 15:34

BurntSushi approved these changes Apr 21, 2017

View reviewed changes

BurntSushi merged commit 362abed into BurntSushi:master Apr 21, 2017

kpp deleted the bug_451_only_mathing_digits branch April 23, 2017 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix reiteration of the first found match with --only-mathing flag #454

Fix reiteration of the first found match with --only-mathing flag #454

kpp commented Apr 20, 2017 •

edited

Loading

BurntSushi left a comment

BurntSushi Apr 20, 2017

kpp Apr 20, 2017

BurntSushi Apr 20, 2017

kpp Apr 20, 2017

BurntSushi Apr 20, 2017

kpp Apr 20, 2017

kpp commented Apr 21, 2017

BurntSushi commented Apr 21, 2017

Fix reiteration of the first found match with --only-mathing flag #454

Fix reiteration of the first found match with --only-mathing flag #454

Conversation

kpp commented Apr 20, 2017 • edited Loading

BurntSushi left a comment

Choose a reason for hiding this comment

BurntSushi Apr 20, 2017

Choose a reason for hiding this comment

kpp Apr 20, 2017

Choose a reason for hiding this comment

BurntSushi Apr 20, 2017

Choose a reason for hiding this comment

kpp Apr 20, 2017

Choose a reason for hiding this comment

BurntSushi Apr 20, 2017

Choose a reason for hiding this comment

kpp Apr 20, 2017

Choose a reason for hiding this comment

kpp commented Apr 21, 2017

BurntSushi commented Apr 21, 2017

kpp commented Apr 20, 2017 •

edited

Loading