Store each path as a Path instead of a String (which is utf8-only)

[bew]

Comment on:

pipe-rename/src/main.rs

Lines 168 to 175 in 5658581

	fn expand_dir(path: &str) -> anyhow::Result<Vec<String>, io::Error> {
	Ok(fs::read_dir(path)?
	.filter_map(|e| e.ok()
	.and_then(|e| e.path()
	.into_os_string()
	.into_string().ok()))
	.collect())
	}

I'm thinking, the filter_map exists only because the conversion from an OsString to a String could fail if the path is not UTF-8 and we might get an Err(...) instead of an Ok(String).

Edit: the filter_map also exists because each entry in from read_dir can be an error, my bad.
My observation is still valid though and is on: .into_string().ok() which will return None for a non-utf8 path, and discard it.

I don't think that's right, the tool shouldn't skip non-utf8 paths.

@marcusbuffett I'm wondering if we could change something in the entire codebase:
--> Instead of storing each path in a String (utf8), we could simply keep it as a Path (which can contain non-utf8).
For the occasional cases where we need to print a path (like for the diff), we could use a replacement char for the non-utf8 chars. With a note for the user at the end of the diff if at least one path is non-utf8, the detection / change could be done before the diff to not over complicate it.

---

NOTE: I'd be interested to take a stab at this if you're ok with this

[marcusbuffett]

@bew i was thinking along the same lines when I updated clap, since you can parse arguments as PathBuf instead of string, but then I ended up having to unwrap stuff to match everything else. I agree we should be working with PathBufs when possible, would love for you to take a stab at it

[marcusbuffett]

Or OsString or whatever, not totally sure, I leave it up to you

[mtimkovich]

That OsString documentation scared me. I'm curious how vim deals with the issue.

…

On Fri, Apr 30, 2021, 5:56 PM Marcus Buffett ***@***.***> wrote: Or OsString or whatever, not totally sure, I leave it up to you — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#30 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE66RMWEZAGOLQFRU3QTJLTLM7TLANCNFSM435NM3AQ> .

[assarbad]

Just thinking out loud: wouldn't a change like this:

1. cause issues across the Windows/non-Windows boundary because of OsString on Windows being 16-bit code units and 8-bit on other systems? OsString to String and back should be lossless for Windows. For other systems this would only be guaranteed for UTF(-8)-based locales, though; i.e. $LANG.
2. move the onus of dealing with the encoding to the invoked command/editor (which may be hard to convey in a generic fashion for anything that's not UTF-8)?