Add a --quote option to put quotation marks around output files/dirs
#1366
Conversation
|
Also FWIW I've never written any rust before yesterday, so I'm pretty confident my code could be improved. Be gentle |
|
I would like to request some help with the unit tests. I have written the tests, but they're not working. Specifically it's not engaging the new |
| fn path_needs_quoting(path: &str) -> i8 { | ||
| // If it contains any special chars we return single quote | ||
| if path.contains(" ") || path.contains("$") || path.contains("\"") { | ||
| return 1; |
There was a problem hiding this comment.
It would be better to use an enumeration instead of an i8, where it isn't obvious what the different numbers mean.
| format!("\"{}\"", tmp_str) | ||
| // No quoting required | ||
| } else { | ||
| path_str.to_string() |
There was a problem hiding this comment.
This unnecessarily copies the string again even if there is no change
|
|
||
| // If the quoted string is new, then we flag that to tweak the offset | ||
| // so the colors line up | ||
| if tmp_str != path_str { |
There was a problem hiding this comment.
This has to compare every character of the string. It would be more efficient if you did something like have return quote_path return an Option that is only Some if quoting happened, and then do:
if let Some(quoted) = quote_path(&path_str) {
path_str = quoted.into();
needs_quoting = true;
}
| @@ -78,6 +128,11 @@ fn print_entry_colorized<W: Write>( | |||
| } | |||
|
|
|||
| if offset > 0 { | |||
|
|
|||
| if needs_quoting { | |||
| offset += 2; | |||
There was a problem hiding this comment.
I don't think this is correct. If it is only quoted shouldn't the offset be just 1 more?
And if it is double quoted, and has escaped characters, then the proper offset will depend on how many characters in the parent ended up getting escaped.
| // Trying to copy: https://www.gnu.org/software/coreutils/quotes.html | ||
| fn path_needs_quoting(path: &str) -> i8 { | ||
| // If it contains any special chars we return single quote | ||
| if path.contains(" ") || path.contains("$") || path.contains("\"") { |
There was a problem hiding this comment.
This is probably not careful enough. See https://github.com/tavianator/bfs/blob/b8ed989642b9f0f6c1301bcff6f1498935cbd81c/src/bfstd.c#L906-L918 for example
|
My code is definitely sub par. This was my proof-of-concept trial. Someone with real Rust experience should tackle this. |
Following the rules set by coreutils - quoting I have added a
--quoteoption to put appropriate quotation marks around files/directories that need them.Note: This fixes #1365