search

cloudhead / rx

👾 Modern and minimalist pixel editor
https://discord.gg/xHggPjfsS9
GNU General Public License v3.0
3.12k stars 109 forks source link

Is there any reason to hand-unwraping paths instead of using `fs::canonicalize` #34

Closed roman-holovin closed 5 years ago

roman-holovin commented 5 years ago

I have tried to use ~, .. in paths and rx is failing to parse those. Also, I suppose it may fail with symlinks also.

https://github.com/cloudhead/rx/blob/75a11ff3667d035a3b673c908ee4409c7dc92d7b/src/parser.rs#L218

There is canonicalize function that should be able to resolve paths more reliably.

cloudhead commented 5 years ago

As far as I know, canonicalize doesn't handle ~, since that is a shell feature. But for relative paths we should be using it, indeed!

rbisewski commented 5 years ago

Yeah, I think cloudhead is right with regards to resolving the ~.

I looked into ways of handling that when it was implemented and the dirs::BaseDirs crate seemed to be best choice since it makes gathering OS-wide / user-wide environment paths fairly easy and safe.

That said, I think implementing relative paths shouldn't be too hard, and I see that even std::path::Path supports an official alias to the canonicalize function as well.

I'll make an attempt at this in a branch later tonight.

roman-holovin commented 5 years ago

Oh, I see. I was confused because documentation mentions that canonicalize uses realpath to expand paths on Unixes and I tried it in the shell and it worked. I missed the fact that shell expansion happens before command execution. Good to know, thanks.

rbisewski commented 5 years ago

Canonicalizing the paths required a bit of a rethink of the path parsing logic function, but I have made a PR that (hopefully) handles this well enough.

rbisewski commented 5 years ago

@cloudhead since this is merged I think this issue could be closed, whenever you have time :)