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
Support for expandable environment names #1024
Support for expandable environment names #1024
Conversation
Previously, LaTeXML only had support for environments which were named literally, i.e. that stated with \begin{somename}. This meant that if an environment name was defined by an expandable token (e.g. \begin\somecs), then LaTeXML would fail. This commit resolves the problem by expanding the argument to \begin and \end. It furthermore adds an appropriate test case to t/expansion/environments.tex.
my $name = $env && ToString($env); | ||
my $before = LookupValue('@environment@' . ToString($_[1]) . '@beforebegin'); | ||
my $after = LookupValue('@environment@' . ToString($_[1]) . '@atbegin'); | ||
my $name = $env && ToString(Expand($env)); |
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.
question: Shouldn't this be realized via the parameter type? E.g. replacing the \begin{}
via an \begin XToken
or such? cc @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.
Also, if $name
is undefined / empty we probably want to throw an error
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.
Well, yeah, sorta, but ... No. There's an Expanded parameter type which we tried to use. It failed because it kinda expands too early(?). Well, really LaTeX gets the argument which might be balanced braces or a single token, eventually that gets expanded. Using Expanded caused latexml to only see the 1st char of the expansion instead of the whole thing; adding extra braces mucked things up differently. It's probably true that one would need (at least) two separate flavors of "expanded"...
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.
Right, it looks like we need a ->readXArg
rather than a ->readXToken
, and since we don't have that yet, the current PR makes sense
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.
But this begs the question if the Expanded
parameter type is ever the correct type to use? Maybe it just needs an upgrade, even separately from the \begin
case.
Currently LaTeXML only has support for environments which are named literally, i.e. those of the form:
However, if an environment name is defined by an expandable token, like in either of the following, LaTeXML fails:
LaTeX itself supports all three cases.
This PR resolves the problem by expanding the argument to
\begin
and\end
. It furthermore adds appropriate test cases tot/expansion/environments.tex
.The solution implemented in this PR is to manually call
Expand()
on the argument. I also tried making the argument of typeDigest
, as well as manuallyDigest()
ing the argument, however both of these cases caused test failures. The same goes for using theExpanded
argument type.