-
Notifications
You must be signed in to change notification settings - Fork 128
use URI::file for (de)serialising paths in XML #2519
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
14f6bc7 to
0a600a7
Compare
| return unless defined $_[0]; | ||
| my @urls = split(/,/, $_[0]); | ||
| my @nonempty = grep { $_ } @urls; | ||
| return map { pathname_from_url($_) } @nonempty; } |
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.
Apologies in advance for the naming nit-picking.
pathname_from_urls probably wants to indicate the many-to-many map. So an extra s -
pathnames_from_urls
Similarly for the _to_ variant.
Also, we probably want to decide if we want to drop the _file from _file_url, or the opposite - to use it everywhere. As indicated, a URL is ambiguous. Indeed a relative pathname on linux such as ./subdir/a/b/c/test.png is also a match for a relative URL.
So maybe all of your new methods should be fileurl/fileurls as suffixes... But maybe don't rush to take my feedback before we hear from @brucemiller .
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.
want to decide if we want to drop the
_filefrom_file_url, or the opposite - to use it everywhere
Additional datapoint: most of the times LaTeXML wants an URL path (e.g. resources, images, CrossRef). The _file_url variant is for DTD and setURI which require actual URLs and may do strange things with absolute URL paths (not sure how, but I wouldn't risk it). Third use is for candidates and search paths in this PR, where we should stick to paths for backwards compatibility.
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.
probably wants to indicate the many-to-many map. So an extra
s-
pathnames_from_urls
Makes sense. I was just uncertain as to whether pathname_ was meant as a namespace as opposed to an actual word.
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.
Looking back at (and rebasing) this PR, a summary of why the PR looks like this:
- the URL without
file:must remain for backward compatibility with external tools that do not expect it - the
file:scheme can be omitted because those 'urls' are used in contexts wherefile:is implied (e.g. graphics candidates) pathname_to_url(s?)is the version withoutfile:scheme, for backward compatibility with the previouspathname_to_url(which was never about producing complete well formed URLs!)pathname_to_file_urlexists only for those rare situations in which thefile:scheme is actually required (DTD and document URI)
So the naming problem really comes from the original pathname_to_url not producing URLs in the first place. We could rename pathname_to_url to pathname_urlencode, which is more accurate.
0a600a7 to
7753463
Compare
Use URI::file for serialising and deserialising paths written to XML for
@graphic,@candidates, and the processing instructions 'searchpaths' and 'graphicspath'. It also replaces previous manual uses of URI::file.This prevents potential issues regarding:
file://when the paths is absolute, including on Windows for paths starting with e.g.C:(or longpaths, but LaTeXML does not support those yet)Note that I have replaced the previous implementation of
pathname_to_urlas it didn't make sense to have two. I have adjusted CrossRef accordingly.This supersedes #2367 and fixes #2355 (well, I haven't tried it on a Windows machine yet, but I am sure it does).