-
Notifications
You must be signed in to change notification settings - Fork 6
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
Handle documents on STDIN and/or via Lua 5.1 #69
base: main
Are you sure you want to change the base?
Conversation
packages/resilient/styles/init.lua
Outdated
@@ -88,7 +88,8 @@ end | |||
|
|||
function package:readStyles () | |||
local yaml = require("resilient-tinyyaml") | |||
local fname = SILE.masterFilename .. '-styles.yml' | |||
local basename = pl.path.splitext(pl.path.normpath(SILE.input.filenames[1])) |
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.
So somehow the magic behind SILE.masterFilename
got broken and 3rd party packages have to use an even weirder magic pl.path.splitext(pl.path.normpath(SILE.input.filenames[1]))
?
I suggest reporting the issue to SILE.
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.
No, it didn't "get broken." It was always broken because it made assumptions about inputs that limited usage. You ran into one such issue yourself in SILE#1833. In working on a fix I ran into several other issues and also raised the point that masterFilename
is not even a file name at all (c.f. SILE#1835). It also has never properly handled STDIN or named pipes (something we added in SILE v0.9.5) because it can't invent a name, and more recently it became obvious that it doesn't play nice with multiple inputs.
We disagreed on what the most useful abstraction was and what to call it, so rather than remove the poorly named and partially functional API we've just left it for now but started using more robust code internally. There is no reason for 3rd party packages to not adopt this until such a time a a standardized base name function of some kind is provided that covers all these bases.
Normalizing a path and getting the base name is not exactly weird magic, but this code is making assumptions about projects using certain file system layouts (and even using real files instead of named pipes, anonymous streams, command substitution, etc.). It's okay to make those assumptions for this use case, but SILE can't magically do the right thing in every case so explicitly doing exactly the thing you want to accomplish here makes sense to me.
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.
My point was always that 3rd party packages should not have to do pl.path.splitext(pl.path.normpath(SILE.input.filenames[1]))
for deriving in a (fairly obscure way) where SILE should have its output.
In sile-typesetter/sile#1835 (comment) you said:
Okay I'll leave masterFilename alone for now so anything 3rd party using it won't trip up.
Still, it now bugs on attempt to concatenate field 'masterFilename' (a nil value)
There is no reason for 3rd party packages to not adopt this until such a time a a standardized base name function of some kind is provided that covers all these bases.
With 15 sile modules on Luarocks, yes there is a reason... I doubt I can cope fast enough with an impredictible release cycle.
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.
My point was always that 3rd party packages should not have to do
pl.path.splitext(pl.path.normpath(SILE.input.filenames[1]))
for deriving in a (fairly obscure way) where SILE should have its output.
You don't have to normalize the path if you don't want to, but I don't understand what you think should happen here. I offered to supply something like a SILE.masterBaseName or something like that and I'm still happy to do so, but I also struggle to see why this seems so hard. Sile gives you the list of inputs as SILE.input.filenames
. If you want to guess at a meta data file that might exist with a similar name, removing the .sil
extension from it with :gsum()
or pl.path.splitext()
is not exactly magic. For many of my use cases that isn't even what I do because I don't want my project directories cluttered up with build artifacts, with CaSILE I place things like this in a completely separate build cache directory with other intermediate artifacts. Maybe your system doesn't involve stripping extensions at all, maybe you just append another suffix.
I guess my point is SILE.input.filenames[1]
seems like a pretty straight forward way to find the first input filename. If you want to derive something from that, go ahead.
In sile-typesetter/sile#1835 (comment) you said:
Okay I'll leave masterFilename alone for now so anything 3rd party using it won't trip up.
Still, it now bugs on
attempt to concatenate field 'masterFilename' (a nil value)
No it doesn't "now bug" ... it still works just the way it has since 0.9.x days which was always incomplete. Your module is inventing a file name and attempting to read it. It's deriving that name from what it assumes will be an input filename, but it never accounted for handling streams where there is no master file name because the input isn't a file.
You've never run into this before probably because you haven't worked in the shell with pipes, and even if you had Lua 5.4 was masking this bug from you by silently casting Nil to a string. Using Lua 5.1 / LuaJIT happens to reveal this bug because it doesn't do that cast and hence shows you when you can't guess a filename based on something that doesn't exist.
There are all sorts of use cases where SILE can be used to render that don't involve either input files or even output files. As a contrived example, lets say you wanted to render a text string to an image and place it here in GitHub as a Markdown image. Running this command:
$ echo -E '\document{\nofolios{}Foo Bar}' | sile -q - -o - | magick -density 300 - -trim png:- | wl-copy
... and then hitting Ctrl+V results in this:
There is no reason for 3rd party packages to not adopt this until such a time a a standardized base name function of some kind is provided that covers all these bases.
With 15 sile modules on Luarocks, yes there is a reason... I doubt I can cope fast enough with an impredictible release cycle.
I don't expect you to cope with everything. This is open source and when I saw a problem that came up in my own usage I submitted a fix for you. If you want to suggest what API SILE should provide I'm happy to go that route, I just didn't want to put work into adding fixes to SILE.masterFilename
when it wasn't even a file name and the actual filename was in SILE.input.filenames
. Again see the related SILE PR. I didn't break anything about masterFilename yet, only suggested it should be superseded by something more correct if we keep anything at all.
I dropped the path normalization function which isn't necessary here and also changed the PR title because my reference to a SILE version seems to have thrown off the discussion. This is not a problem that started with SILE v0.14.11, it only fixes a bug in this library by making use of the more consistent |
Possibly affected modules (using
|
The concern with embedders, printoptions, labelrefs and the ToC is a bit different from that of resilient styles
|
Hmm. In the above sense, styles are perhaps incompatible with content from STDIN. Perhaps this workflow should just be rejected... |
These classes seem to work fine when the style or other possible external files are not found. I would still make this change and just let it not find the appropriate file and skip it rather than crashing on not being able to guess a filename. |
I noticed while checking my suggestions in this comment that resilient is tripping up on documents coming from STDIN, e.g.:
This should fix that by doing the same thing SILE is internally, but I haven't actually tested it.