add line-range #162

tskinn · 2018-06-01T03:15:34Z

This is my first attempt at actually writing some rust code so bear with me.

I wasn't sure of the best place to put the logic for checking line range. I started with putting it in the print_line function but moved it out to be able to short circuit and stop processing once out of the range.

I didn't add the ability to do multiple ranges yet but it should be fairly simple.

How robust of error checking do we need on validating the line ranges? Like do we want to sort or make sure they are not conflicting or that they are in least to greatest order or anything like that?

sharkdp

Thank you very much!

I added some inline comments. You can leave out the error handling part at first, if you want. This will probably be quite challenging if you are not (yet) too familiar with Rust.

I wasn't sure of the best place to put the logic for checking line range. I started with putting it in the print_line function but moved it out to be able to short circuit and stop processing once out of the range.

👍 This way we can also easily write unit tests (which we should add before merging this).

I didn't add the ability to do multiple ranges yet but it should be fairly simple.

Okay. Let's leave this out for now and add in a second PR.

How robust of error checking do we need on validating the line ranges? Like do we want to sort or make sure they are not conflicting or that they are in least to greatest order or anything like that?

I think we can also silently ignore it if someone puts --line-range 9:3. They won't get any output. Showing an error would also be fine for me.

As for multiple ranges, I think they cannot be conflicting because we would add the ranges, right? --line-range 3:5,7:9 would show lines 3,4,5,7,8,9. --line-range 3:7,5:9 would show all lines from 3 to 9.

sharkdp · 2018-06-01T07:39:55Z

src/app.rs

+                    .help("Print range of lines")
+                    .long_help(
+                        "Print a specified range of lines from the files \
+                         --line-range 30-40 or --line-range 30-",


maybe: "for example '--line-range 30-40' or .."?

sharkdp · 2018-06-01T07:41:16Z

src/app.rs

@@ -269,6 +280,7 @@ impl App {
            term_width: Term::stdout().size().1 as usize,
            files,
            theme: self.matches.value_of("theme"),
+            line_range: get_ranges(self.matches.value_of("line-range")),


I would write this as self.matches.value_of("line-range").map(get_ranges). This would allow us to get rid of the outermost match in get_ranges. The signature would change to fn get_ranges(value: &str) -> ...

sharkdp · 2018-06-01T07:47:05Z

src/app.rs

 }

 fn is_truecolor_terminal() -> bool {
    env::var("COLORTERM")
        .map(|colorterm| colorterm == "truecolor" || colorterm == "24bit")
        .unwrap_or(false)
 }
+
+fn get_ranges(value: Option<&str>) -> Option<(usize, usize)> {


A few things:

Maybe rename this to parse_ranges?

This should probably return a Result<...> in order to properly handle the errors that may occur.

The Option<..> is probably not necessary if we use .map(get_ranges), as suggested above.

sharkdp · 2018-06-01T07:52:46Z

src/app.rs

@@ -322,10 +334,37 @@ pub struct Config<'a> {
    pub term_width: usize,
    pub files: Vec<Option<&'a str>>,
    pub theme: Option<&'a str>,
+    pub line_range: Option<(usize, usize)>,


I think it would be worth to write a small struct LineRange that instead of (usize, usize). The get_ranges/parse_ranges function could then be a static LineRange::new or LineRange::from function.

I think it would also be cleaner to represent the open bounds as None instead of min_value/max_value:

struct LineRange { lower: Option<usize>, upper: Option<usize>, } impl LineRange { pub fn new(value: &str) -> Self { // ... } }

This might also complicate some things, so I'm not quite sure.

sharkdp · 2018-06-01T07:57:01Z

src/app.rs

+        None => None,
+        Some(str_range) => {
+            let mut new_range = (usize::min_value(), usize::max_value());
+            if str_range.bytes().nth(0).expect("Something should be here!") == b':' {


We typically try to avoid expect or unwrap because these methods will panic (i.e. immediately abort the program) if the input is not as expected, preventing any potential cleanup operations from happening. The safe way to do this is via Result<..>, as suggested above (see https://doc.rust-lang.org/book/second-edition/ch09-02-recoverable-errors-with-result.html - in particular the usage of the ? operator).

The help text uses n-m, but here you seem to implement n:m?

sharkdp · 2018-06-01T07:59:18Z

src/features.rs

    while reader.read_until(b'\n', &mut buffer)? > 0 {
        {
            let line = String::from_utf8_lossy(&buffer);
            let regions = highlighter.highlight(line.as_ref());

-            printer.print_line(&regions)?;
+            if printer.config.line_range.is_some() {


You should probably match on the Option here in order to avoid the unwrap() calls below.

tskinn · 2018-06-02T19:12:24Z

I appreciate the feedback. I will start working on this. Thanks for you patience. Feel free to let me know if you think I'm going in the completely wrong direction with this.

tskinn · 2018-06-04T23:25:29Z

@sharkdp I changed the code quite a bit and learned enough rust to try to implement some of the things you mentioned, but I wan't able to implement it exactly like you suggested in a few cases. I kept running in circles trying to fit your different suggestions together with my limited knowledge of rust but it is working!

I would appreciate more feedback and guidance. I'm sure there are better ways to do some of these things and probably some style changes are needed.

sharkdp

Thank you very much for these great changes and for your persistence!

sharkdp · 2018-06-06T19:24:26Z

src/app.rs

+}
+
+impl LineRange {
+    pub fn from(range_raw: &str) -> LineRange {


I think this should also return a Result<LineRange> (by removing the unwrap_or call below). This way, we can pass the error messages through to the call-site.

sharkdp · 2018-06-06T19:26:21Z

src/app.rs

+    pub fn parse_range(range_raw: &str) -> Result<LineRange> {
+        let mut new_range = LineRange::new();
+
+        if range_raw.bytes().nth(0).ok_or("No first byte")? == b':' {


Maybe the error message here (and below) should be "Empty line range"?

sharkdp · 2018-06-06T19:27:29Z

src/app.rs

+        }
+
+        let line_numbers: Vec<&str> = range_raw.split(':').collect();
+        if line_numbers.len() == 2 {


I think we should have an else branch here which returns a error ("expected single ':' character").

This might be a silly question but how do I go about creating an error? When I try to return
Err("expected single....") I get a mismatched types error: expected struct `errors::Error`, found reference

Do I need to implement a new error type or something like that?

You should be able to use Err("expected single....".into()).

sharkdp · 2018-06-06T19:28:14Z

src/app.rs

+
+#[test]
+fn test_parse_line_range_full() {
+    let range = LineRange::from(Some("40:50")).expect("Shouldn't fail on test!");


This would turn into ::from("40:50").

Does this only work if the tests are in a new module called LineRange?

Ah, sorry. I just meant to say that the Some part will not be needed anymore. I would still use LineRange::from.

Oh duh. Gotcha

sharkdp · 2018-06-06T19:28:41Z

src/app.rs

+    let range = LineRange::from(Some("40:")).expect("Shouldn't fail on test!");
+    assert_eq!(40, range.lower);
+    assert_eq!(usize::max_value(), range.upper);
+}


If we change the signature of LineRange::from to return a Result, we can also add some tests for negative cases.

sharkdp · 2018-06-06T19:29:39Z

src/features.rs

-        None => Box::new(stdin.lock()),
-        Some(filename) => Box::new(BufReader::new(File::open(filename)?)),
-    };
+fn print_file_ranges<'a>(


It would be really great if we had just a single function and would avoid this code duplication. I would try to change the ranges parameter to accept an Option<Range>.

sharkdp · 2018-06-06T19:30:03Z

src/main.rs

@@ -41,6 +41,7 @@ mod errors {
            Clap(::clap::Error);
            Io(::std::io::Error);
            SyntectError(::syntect::LoadingError);
+            ParseIntError(::std::num::ParseIntError);


sharkdp · 2018-06-06T19:56:39Z

src/app.rs

@@ -269,6 +282,7 @@ impl App {
            term_width: Term::stdout().size().1 as usize,
            files,
            theme: self.matches.value_of("theme"),
+            line_range: self.matches.value_of("line-range").map(LineRange::from),


If we change the signature of LineRange::from, as suggested above, we could use

line_range: tranpose(self.matches.value_of("line-range").map(LineRange::from))

such that the parsing-error-messages would bubble up to the main function (and reported to the user).

Here, transpose is a little helper function that turns a "Option of Result" into a "Result of Option". It could be implemented like this:

fn transpose<T, E>(opt: Option<Result<T, E>>) -> Result<Option<T>, E> { opt.map_or(Ok(None), |res| res.map(Some)) }

This function is actually an experimental feature which means that we could soon replace this by a call to the standard-library .transpose(): https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.transpose

OK I think I see what you are saying now. We want to bubble up any errors that happen when parsing the linerange to bubble up from the config() function.

sharkdp

Thank you very much!

add line-range

7489249

sharkdp requested changes Jun 1, 2018

View reviewed changes

tskinn added 2 commits June 4, 2018 17:07

update line-ranges

e26e33b

use map()

79a6ab1

sharkdp requested changes Jun 6, 2018

View reviewed changes

update

6e1c6e4

sharkdp approved these changes Jun 12, 2018

View reviewed changes

sharkdp added 2 commits June 12, 2018 07:49

Run 'cargo fmt'

f14496a

Change help text for --line-range

fd5b4c3

sharkdp changed the title ~~[WIP] add line-range~~ add line-range Jun 12, 2018

sharkdp merged commit 65e6970 into sharkdp:master Jun 12, 2018

sharkdp mentioned this pull request Jun 12, 2018

Print Range of Lines #159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add line-range #162

add line-range #162

tskinn commented Jun 1, 2018

sharkdp left a comment

sharkdp Jun 1, 2018

sharkdp Jun 1, 2018

sharkdp Jun 1, 2018

sharkdp Jun 1, 2018

sharkdp Jun 1, 2018

sharkdp Jun 1, 2018

tskinn commented Jun 2, 2018

tskinn commented Jun 4, 2018

sharkdp left a comment

sharkdp Jun 6, 2018

sharkdp Jun 6, 2018

sharkdp Jun 6, 2018

tskinn Jun 9, 2018

sharkdp Jun 10, 2018

sharkdp Jun 6, 2018

tskinn Jun 9, 2018

sharkdp Jun 10, 2018

tskinn Jun 12, 2018

sharkdp Jun 6, 2018

sharkdp Jun 6, 2018

sharkdp Jun 6, 2018

sharkdp Jun 6, 2018 •

edited

Loading

tskinn Jun 9, 2018

sharkdp left a comment

add line-range #162

add line-range #162

Conversation

tskinn commented Jun 1, 2018

sharkdp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tskinn commented Jun 2, 2018

tskinn commented Jun 4, 2018

sharkdp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkdp Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkdp left a comment

Choose a reason for hiding this comment

sharkdp Jun 6, 2018 •

edited

Loading