lunarmodules / luafilesystem

LuaFileSystem is a Lua library developed to complement the set of functions related to file systems offered by the standard Lua distribution.
https://lunarmodules.github.io/luafilesystem/
MIT License
914 stars 292 forks source link

pl.Path.isdir doesn't work properly if the path prefix start by "~" #172

Closed AllenFang closed 1 year ago

AllenFang commented 1 year ago

when a given path is something like "~/User/data.csv" for example, the isdir and isfile seems work incorrect. guessing the problem is luafilesystem can't handle the ~ well?

thanks

alerque commented 1 year ago

This does seem to trace back to lfs.attributes not returning 'directory' so I migrated the issue to this repository. If fixed in the lfs C module then it should trickle down to work correctly in Penlight with no updates there.

Tieske commented 1 year ago

lfs doesn't handle this. You can use Penlight for this; https://lunarmodules.github.io/Penlight/libraries/pl.path.html#expanduser

Or if you need to, copy the implementation; https://github.com/lunarmodules/Penlight/blob/master/lua/pl/path.lua#L492-L507

alerque commented 1 year ago

What do you think about making LFS handle this? It seems like making it aware of different path segments like this makes sense.

Tieske commented 1 year ago

not sure. lfs has a wider scope I think (platforms), and less assumptions. Isn't the "~" thing a Unix only thing? which just happens to work on Windows within Penlight. Not sure though. But that's the kind of stuff I'd not like to add to lfs.

AllenFang commented 1 year ago

my two cents is if we don't fix this problem in LFS, every develop they use penlight path for ~ cases, they have to wrap up the path by expandUser always to make sure the path got resolve correctly. So It sounds like if we can fix in LFS, then penlight path will have a consistent behavior and experience. User won't need to care if their path got such ~ segment or not.

alerque commented 1 year ago

Yes ~ is a Unix(ish) only thing, but it is a Unix(ish) thing. It isn't some ad-hoc extension used sometimes, it is standardized and practically universal .... to the point where when it doesn't work it's almost always perceived as a bug. Which is why we're here. The answer LFS is returning for attributes("~") is objectively wrong. Just like attributes("D:\foo") on Windows should resolve to a specific drive and location attributes("~/foo") should resolve properly for Unix.

One could debate whether a shim to also make it a valid way to resolve user profile directories on Windows, but at least on platforms where it is a valid path, parsing it the way the platform handles paths seems the only consistent approach to me.

Note also that most shells and Unix utilities require escaping the ~ character if meant as a literal (if they even have such a mechanism), meaning in the event we really did not want to resolve it, we're probably still doing the wrong thing by resolving it to a literal path as is.

Tieske commented 1 year ago

I have no big objections to it, but... should some other stuff like normalization of paths then also be added? Maybe we should check the Penlight functions in the pl.path module to check which ones make sense to add to lfs then?

Tieske commented 1 year ago

@hishamhm @ignacio any inputs? since you are the listed maintainers of this repo.

hishamhm commented 1 year ago

~ is shell expansion. It should be performed by the shell only and applications must not get in the way. If an user has a filename containing a ~ they can escape it in the shell and pass it to the application. The application must not try to expand it again. So, it's a firm no for me on this feature request, sorry.

On Tue, Nov 21, 2023, 19:05 Thijs Schreijer @.***> wrote:

@hishamhm https://github.com/hishamhm @ignacio https://github.com/ignacio any inputs? since you are the listed maintainers of this repo.

— Reply to this email directly, view it on GitHub https://github.com/lunarmodules/luafilesystem/issues/172#issuecomment-1821774749, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB365OJHPJIKQ5VYUKF76DYFUQRRAVCNFSM6AAAAAA7RVV6SCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRRG43TINZUHE . You are receiving this because you were mentioned.Message ID: @.***>

alerque commented 1 year ago

While I could counter with a couple hundred non-shell user space utilities, I can see the argument for it being expansion of some kind and any sort brings the function into a different territory.

@allen I guess that means your answer is:

Expand with any expansion functions you want to support first (pl.path.expanduser() having already been mentioned) and then run whatever checks you want on it.

AllenFang commented 1 year ago

alright, thanks @alerque 🙇

hishamhm commented 1 year ago

To further clarify, there's also the portability aspect: ~ and wildcards such as *.c are performed by shell expansion and not by the apps themselves on Unix... tildes and wildcards arrive already expanded in the process's argv on Unix. On MS-DOS and its derivatives, however, it's the opposite: to the best of my memory, expansions are handled by the apps and not by COMMAND.COM or CMD.EXE (CMD has some really funky syntax to the FOR command for performing some expansions in the shell if I recall correctly). Given that luafilesystem is portable across Unix and Windows-based systems, that is yet another reason to not bundle this OS-specific concern to the library. And to echo what others have said, using Penlight for that if you really need to do tilde-expansion in-app is a fine choice.

On Wed, Nov 22, 2023, 04:11 Caleb Maclennan @.***> wrote:

While I could counter with a couple hundred non-shell user space utilities, I can see the argument for it being expansion of some kind and any sort brings the function into a different territory.

@allen https://github.com/allen I guess that means your answer is:

Expand with any expansion functions you want to support first ( pl.path.expanduser() having already been mentioned) and then run whatever checks you want on it.

— Reply to this email directly, view it on GitHub https://github.com/lunarmodules/luafilesystem/issues/172#issuecomment-1822224869, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB365LVC7ZFHOG6MZ5FQULYFWQTDAVCNFSM6AAAAAA7RVV6SCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRSGIZDIOBWHE . You are receiving this because you were mentioned.Message ID: @.***>