-
Notifications
You must be signed in to change notification settings - Fork 128
move (almost) all I/O under Util::Pathname #2640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
760ee73 to
2820f15
Compare
|
This is very scary, but I really like the idea of collecting all the IO stuff into one (hopefully, eventually) consistent API. Should also help to finally focus on dealing with some if the Issues & PR's that you've worked on regarding portability. We'll definitely want to think through the naming conventions, though. And we'll need a lot of feedback from @dginev, when he's available. [In the meantime: Thanks for all the effort!] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am lightly concerned about the performance loss (many new function calls), but if the consistency is worthwhile that is likely fine. We only manage on the order of hundreds of files in a single conversion run.
The names of the new functions are fine actually - they fit the other pathname_* functions. If we wanted to be explicit that these are file test we can consider adding a _filetest_ piece. So pathname_filetest_r. Or we can go even shorter and do filetest_r? But if we were to add a new term, I'd use the one from the perl documentation.
|
Also, could you please rebase the PR to the master branch? Windows CI should be passing again. |
dginev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Circling back here (while Bruce is still "on holiday"), I still think we need a better naming strategy for the testing subroutines.
pathname_f just doesn't evoke any useful meaning to me. filetest_f does - so maybe that is a reasonable change? Or a name on those lines?
I mean that for the subs that do the perl -dash tests, specifically.
|
I don't have deep knowledge of the code, but I did notice this PR. Could some of this be done by bringing in It won't solve the encoding problem, but it may help with the API design. |
In the meanwhile I changed it to |
This is an attempt at refactoring all I/O calls that deal with file/directory names through wrapper functions in Util::Pathname. This means all the
-Xtests,open,opendir,stat, andunlink. There might be others I did not catch.My goal is to guarantee that normal LaTeXML code is never exposed to raw file names, which are not Unicode strings and must be decoded and encoded before use. The enconding and decoding will be done in the wrapper functions, although not yet – I first want to know if this refactor is ok.
(The encoding and decoding itself is simple, see #2545 for Unix using Encode::Locale. For Windows, I concluded that the only way to go is Win32::LongPath.)