Filter a log file by matching multiple regular expressions #251

yandexx · Jul 19, 2017

For Issue #244

A few concerns I have:

Is it even a good example to only filter by a couple words?
I used RegexSetBuilder instead of RegexSet, I think it's a friendlier way to force case insensitive search and shows the builder pattern.
Is it potentially confusing to use shadowing there with let line = line?;

yandexx · Jul 19, 2017

OK the test is also failing since there's no actual log file. Should I then create a several line bogus log file at the beginning of the example and hide it away?

budziq · Jul 19, 2017

Hi @yandexx thanks for all the submissions!

Unfortunately I'll not be able to follow up with anything resembling review for the next two weeks due to holidays 😞 . I may drop some comments when Im on mobile later and I've asked other maintainers to drop from time to time and look into the PR's

budziq · Jul 19, 2017

budziq requested changes Jul 19, 2017

View changes

@yandexx Very nice!

After obligatory disclaimer about low quality of this review here are some of my thoughts below:

Also no need to add any bogus log (please see comment about no_run below.
I hope that I've answered most of your questions below.

budziq · Jul 19, 2017

src/basics.md

+extern crate regex;
+
+# use std::fs::File;
+# use std::io::{BufReader, BufRead};


I would not hide these use statements. We generally hide only the error boilerplate or support code that is far from the point.

budziq · Jul 19, 2017

src/basics.md

+
+Reads a file named `application.log` and only outputs the lines 
+containing “warning” or “error” (case insensitive). A [`regex::RegexSet`] is built
+with [`regex::RegexSetBuilder`] using the builder pattern.


using the builder pattern

I would remove this last part of sentence. The RegexSetBuilder indicates it already.

budziq · Jul 19, 2017

src/basics.md

+containing “warning” or “error” (case insensitive). A [`regex::RegexSet`] is built
+with [`regex::RegexSetBuilder`] using the builder pattern.
+
+```rust


make the example no_run to avoid test fail but still check for compilation.

budziq · Jul 19, 2017

src/basics.md

+                .build()?;
+
+    for line in buffered.lines() {
+        let line = line?;


in general shadowing is ok but i would suggest using iterator combinators (eg filter_map) to avoid bailing out on first error. Please see the following example for details.

budziq · Jul 19, 2017

src/basics.md

+                .case_insensitive(true)
+                .build()?;
+
+    for line in buffered.lines() {


at least one if not both log_file / buffered variables can be removed if you instantiate BufferedReader here with File::Open as an argument.

Please note if the code becomes not legible due to sone of the suggested changes feel free to disregard it.

budziq · Jul 19, 2017

src/basics.md

+    let log_file = File::open(log_path)?;
+    let buffered = BufReader::new(log_file);
+
+    let set = RegexSetBuilder::new(&[r"warning", r"error"])


how about adding more complicated regexes here to make the example more real life. Just think of the last loggs you have greped and construct something similar. You can go wild here ;)

budziq · Jul 20, 2017

budziq requested changes Jul 20, 2017

View changes

Nice some final cleanup and squash to single commit and we are ready to merge 👍

budziq · Jul 20, 2017

src/basics.md

-```rust
+A [`regex::RegexSet`] is built with [`regex::RegexSetBuilder`].
+Since backslashes are very common in regular expressions, using
+[raw string literals][raw-string-literals] make them more readable.


no need for link renaming here. you can define the link directly as you would like it displayed at the footer.

[raw string literals]: https://doc.rust-lang.org/reference/tokens.html#raw-string-literals

budziq · Jul 20, 2017

src/basics.md

+    let log_path = "application.log";
+    let buffered = BufReader::new(File::open(log_path)?);
+
+    let set = RegexSetBuilder::new(&[r#"version "\d\.\d\.\d""#, r#"\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}:443"#])


please break the line into shorter ones and run rustfmt on the example.
also please add some third regex for good measure (how about Some warning and some specified text later on?)

budziq · Jul 20, 2017

src/basics.md

+
+use std::fs::File;
+use std::io::{BufReader, BufRead};
+


no need for a newline here

budziq · Jul 20, 2017

budziq approved these changes Jul 20, 2017

View changes

Looks good to me! Unfortunately merging will have to wait for another maintainer or my return from holidays (I cannot perform few remaining sanity checks on mobile ;) )

yandexx · Jul 20, 2017

Thanks, glad to be of help!

brson · Jul 25, 2017

Thanks @yandexx

Filter a log file by matching multiple regular expressions
Filter a log file by matching multiple regular expressions Better queries, added filter_map etc. Ran through rustfmt, added another regex

Loading status checks…

e8aa408

yandexx force-pushed the yandexx:regex-filter-log branch from ab99414 to e8aa408 Jul 20, 2017

brson merged commit 8e6e65d into rust-lang-nursery:master Jul 25, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

yandexx deleted the yandexx:regex-filter-log branch Jul 25, 2017

rust-lang-nursery/rust-cookbook

Filter a log file by matching multiple regular expressions #251

Filter a log file by matching multiple regular expressions #251

yandexx commented Jul 19, 2017

This comment has been minimized.

yandexx commented Jul 19, 2017

This comment has been minimized.

budziq commented Jul 19, 2017

budziq requested changes Jul 19, 2017

View changes

budziq left a comment •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

budziq requested changes Jul 20, 2017

View changes

budziq left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

yandexx force-pushed the yandexx:regex-filter-log branch from `ab99414` to `e8aa408` Jul 20, 2017

budziq approved these changes Jul 20, 2017

View changes

budziq left a comment

This comment has been minimized.

yandexx commented Jul 20, 2017

brson merged commit `8e6e65d` into rust-lang-nursery:master Jul 25, 2017
1 check passed

1 check passed

This comment has been minimized.

brson commented Jul 25, 2017

yandexx deleted the yandexx:regex-filter-log branch Jul 25, 2017

rust-lang-nursery/rust-cookbook

Join GitHub today

Filter a log file by matching multiple regular expressions #251

Conversation

yandexx commented Jul 19, 2017

This comment has been minimized.

yandexx commented Jul 19, 2017

This comment has been minimized.

budziq commented Jul 19, 2017

budziq requested changes Jul 19, 2017 View changes

budziq left a comment • edited

This comment has been minimized.

budziq Jul 19, 2017

This comment has been minimized.

budziq Jul 19, 2017

This comment has been minimized.

budziq Jul 19, 2017

This comment has been minimized.

budziq Jul 19, 2017

This comment has been minimized.

budziq Jul 19, 2017 • edited

This comment has been minimized.

budziq Jul 19, 2017 • edited

budziq requested changes Jul 20, 2017 View changes

budziq left a comment

This comment has been minimized.

budziq Jul 20, 2017

This comment has been minimized.

budziq Jul 20, 2017

This comment has been minimized.

budziq Jul 20, 2017

yandexx force-pushed the yandexx:regex-filter-log branch from ab99414 to e8aa408 Jul 20, 2017

budziq approved these changes Jul 20, 2017 View changes

budziq left a comment

This comment has been minimized.

yandexx commented Jul 20, 2017

Hide details View details brson merged commit 8e6e65d into rust-lang-nursery:master Jul 25, 2017 1 check passed

1 check passed

This comment has been minimized.

brson commented Jul 25, 2017

yandexx deleted the yandexx:regex-filter-log branch Jul 25, 2017

budziq requested changes Jul 19, 2017

View changes

budziq left a comment •

edited

budziq Jul 19, 2017 •

edited

budziq Jul 19, 2017 •

edited

budziq requested changes Jul 20, 2017

View changes

yandexx force-pushed the yandexx:regex-filter-log branch from `ab99414` to `e8aa408` Jul 20, 2017

budziq approved these changes Jul 20, 2017

View changes

brson merged commit `8e6e65d` into rust-lang-nursery:master Jul 25, 2017
1 check passed