Skip to content
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

Merged
merged 10 commits into from
Dec 11, 2024
Merged

Rearrange3 #2442

merged 10 commits into from
Dec 11, 2024

Conversation

brucemiller
Copy link
Owner

Another batch of fidelity improvements, compatibility with latex.ltx, code cleanup and sensible tests.

@brucemiller brucemiller requested a review from dginev November 29, 2024 20:27
Copy link
Collaborator

@dginev dginev left a 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 ~
Copy link
Collaborator

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.

Copy link
Owner Author

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).

Copy link
Collaborator

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.

Copy link
Owner Author

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.

Copy link
Collaborator

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:

  1. with pdftex it is macro:->\penalty \@M \
  2. 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.

Copy link
Owner Author

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.

Copy link
Collaborator

@dginev dginev Dec 1, 2024

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.

@brucemiller brucemiller merged commit 7418141 into master Dec 11, 2024
26 checks passed
@brucemiller brucemiller deleted the rearrange3 branch December 11, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants