-
Notifications
You must be signed in to change notification settings - Fork 101
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
Rearrange3 #2442
Rearrange3 #2442
Conversation
…space); add bindings; update tests
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.
All looks well, good to merge.
Left a minor comment about (accidentally?) dropping one test case.
@@ -15,7 +15,10 @@ | |||
\meaning% | |||
|
|||
|
|||
\meaning ~ | |||
%\meaning ~ |
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.
Why remove the \meaning ~
test? I suspect we can keep it.
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 the commit message was too cryptic: As far as testing \meaning
of an active char, the replacement tests just as well. But ~
expands to something a bit too low level (eg. \nobreakspace{}
; diff in tex & latex) and we really want to keep ~
as something revertable (eg. primitive, which makes a jarring difference in the pdf & xml tests).
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.
Ok, but then we can just keep the test and update the XML to have that case output \lx@NBSP
, right? I think I'd prefer that to removing it, as it both tracks the change and documents an interesting low-level difference.
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 can sympathize with the desire to document those kinds of differences, but there are so many, why single out this one in a testcase for \meaning
? Any primitive or constructor that's not a raw TeX primitive should be a macro? Perhaps a test case dedicated to recording all the "official" \lx@internal
cs? I worry that enshrining \lx@NBSP
here may tend to make testing more, rather than less, confusing.
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.
Splitting it out in a new test file with other related cases sounds great actually.
As you say, it is an interesting case where \meaning
originally returns different values based on the engine:
- with pdftex it is
macro:->\penalty \@M \
- with pdflatex it is
macro:->\nobreakspace {}
Using the tests to document what latexml does today seems useful - and helps track when/where that changes.
The main reason I noticed it here is that it used to be tested and got removed. Having more tests is very helpful while porting, so it is easy for me to lean towards instead moving and expanding the test coverage.
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.
Such a test may be useful, but as I'm still in the midst of regularizing such internals, it may be a bit early.
And, although it's challenging to make tests more unit-like, since "Everything is Connected", and you want to test the interdependences, I've been spending an inordinate amount of time debugging gratuitous changes to tests that aren't really supposed to be testing the thing that randomly changed.
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.
Alright, let's take a rain check on this. I'll try to remember bringing up test reorganization when v0.8.9 release work starts.
Another batch of fidelity improvements, compatibility with latex.ltx, code cleanup and sensible tests.