Add path-separator option. #429

mookid · 2019-04-10T22:33:44Z

Example usage: fd.exe --path-separator / on windows.

add tests
measure performance impact without path-separator option
measure performance impact with path-separator option

benchmarks

Average over 15 runs (with warm cache), using the command fd . %userprofile% (full results):

	feature, sep=default	feature, sep='='	master
avg	5942.6667	6363.6000	5885.4000
std	105.8730	109.2185	115.7299
std / avg	0.0178	0.0172	0.0197

compared to master	1.01	1.08	1.00

without changing the separator, the change does not impact performance; when changing it, it seems to be ~8% slower.

see #428

Example usage: `fd.exe --path-separator /` on windows.

sharkdp

Thank you very much for your contribution!

A few general comments (in addition to the inline comments):

Printing paths is not the only place where we deal with paths. We should search the whole code base for std::path::MAIN_SEPARATOR and see if something needs to be done.
I would like to see a few tests that check that everything works as expected.
We should also perform some benchmarks to see that this does not have any impact on performance (if --path-separator is not used) and to check by how performance degrades when -path-separator is used.

sharkdp · 2019-04-11T19:16:53Z

src/app.rs

+            arg("path-separator")
+                .takes_value(true)
+                .long("path-separator")
+                .number_of_values(1),


is this needed?

Honestly, I don't really know what I am doing here; I am inexperienced with clap and tried to find something that works ok.

src/app.rs

sharkdp · 2019-04-11T19:21:44Z

src/app.rs

+    doc!(h, "path-separator"
+        , "Set the path separator to use when printing file paths."
+        , "Set the path separator to use when printing file paths. \
+           A path separator is limited to a single byte.");


Maybe "limited to a single character" would be better for users that don't immediately associated 1 byte with 1 character. I know that it's probably not quite correct technically (multi-byte characters).

That makes me wonder: why not allow for general Unicode characters? I would change u8 to char and change the help text to:

Set the path separator to use when printing file paths. The separator must be a single character like '/' or '\'.

sharkdp · 2019-04-11T19:24:49Z

src/internal/opts.rs

@@ -75,4 +75,7 @@ pub struct FdOptions {

    /// Whether or not to display filesystem errors
    pub show_filesystem_errors: bool,
+
+    /// The separator used to print file paths.
+    pub path_separator: Option<u8>,


why not char (see above)

sharkdp · 2019-04-11T19:25:34Z

src/main.rs

@@ -113,6 +113,20 @@ fn main() {
        _ => atty::is(Stream::Stdout),
    };

+    let path_separator: Option<u8> = match matches.value_of("path-separator") {


Is the type annotation needed here?

sharkdp · 2019-04-11T19:26:53Z

src/main.rs

+            let sep = sep_str.as_bytes();
+            if sep.len() != 1 {
+                print_error_and_exit!(
+                    "'{}' is not a valid path separator. See 'fd --help'.",


Instead of referring to the help text, maybe just say explicitly what the problem is: "The argument to --path-separator must be a single character".

sharkdp · 2019-04-11T19:29:52Z

src/main.rs

+            }
+            Some(sep[0])
+        }
+        None => None,


Actually, I think that case can not happen (since you require --path-separator to have a value). If this is the case, I think you can simply unwrap the Option:

matches.value_of("path-separator").except("path-separator")

I confirm that not passing the argument triggers the None case.

sharkdp · 2019-04-11T19:35:15Z

src/output.rs

+
+        match config.path_separator {
+            None => write!(stdout, "{}", style.paint(path_string))?,
+            Some(sep) => {


A couple of things:

This should probably be in a separate function

I don't think this is the best way to solve this (convert to byte vector, loop over vector, convert to string). Why not use a string-replacement function?

Instead of checking for cfg!(windows), this code should replace std::path::MAIN_SEPARATOR with config.path_separator. Otherwise, on Windows, you would replace both / and \ with the new separator.

If we are to use a string replacement, we can also handle any string replacement; it would make the code simpler. Do you have anything against it?

mookid · 2019-04-12T00:33:55Z

Printing paths is not the only place where we deal with paths. We should search the whole code base for std::path::MAIN_SEPARATOR and see if something needs to be done.

I am a bit confused: this PR deals only with the presentation logic, that is why the changes are bound to output.rs + command line arguments.

As far as I can tell the rest of the logic has no issue on windows.

mookid · 2019-04-14T17:52:08Z

@sharkdp I think I addressed most points. Tell me if you think anything should be added.

mookid · 2019-04-26T18:39:28Z

re @sharkdp

do you think that anything prevents merging?

mookid · 2019-09-02T21:48:27Z

ping!

sharkdp

Thank you very much!

Add path-separator option.

7602fa9

Example usage: `fd.exe --path-separator /` on windows.

mookid force-pushed the path-separator branch from f8fe926 to 7602fa9 Compare April 10, 2019 22:57

sharkdp requested changes Apr 11, 2019

View reviewed changes

mookid added 4 commits April 12, 2019 00:46

Amend clap configuration for path-separator.

f4fc52d

Handle any string replacement.

3c2c00d

Factor into a helper function; adapt to print_entry_uncolorized.

d452e82

Remove type annotation.

c504124

Add a non regression test.

117c95d

mookid closed this May 17, 2019

vvuk mentioned this pull request Aug 7, 2019

Support ripgrep's --path-separator option #428

Closed

mookid reopened this Aug 10, 2019

sharkdp added 2 commits September 15, 2019 10:36

Extend help text

6b7055e

Use existing Cow pointer

293b0b7

sharkdp approved these changes Sep 15, 2019

View reviewed changes

sharkdp merged commit f14b854 into sharkdp:master Sep 15, 2019

fawick mentioned this pull request Feb 20, 2020

Option --path-separator failing on MSys (Git Bash) #537

Closed

claudetete mentioned this pull request May 27, 2020

Option --path-separator does not work on pattern with --full-path #607

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add path-separator option. #429

Add path-separator option. #429

mookid commented Apr 10, 2019 •

edited

Loading

sharkdp left a comment

sharkdp Apr 11, 2019

mookid Apr 11, 2019

sharkdp Apr 11, 2019

sharkdp Apr 11, 2019

sharkdp Apr 11, 2019

sharkdp Apr 11, 2019

sharkdp Apr 11, 2019

mookid Apr 11, 2019

sharkdp Apr 11, 2019

mookid Apr 11, 2019

mookid commented Apr 12, 2019

mookid commented Apr 14, 2019

mookid commented Apr 26, 2019

mookid commented Sep 2, 2019

sharkdp left a comment

Add path-separator option. #429

Add path-separator option. #429

Conversation

mookid commented Apr 10, 2019 • edited Loading

benchmarks

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

mookid commented Apr 12, 2019

mookid commented Apr 14, 2019

mookid commented Apr 26, 2019

mookid commented Sep 2, 2019

sharkdp left a comment

Choose a reason for hiding this comment

mookid commented Apr 10, 2019 •

edited

Loading