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

fix: issues parsing with 'preserveLatex' set to 'true' #212

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

samueltlg
Copy link

@samueltlg samueltlg commented Oct 24, 2024

Noticed that verbatimLatex not being saved on BoxedExpression instances created from ce.parse for some time. Looks as if this traceable to commits:

cd557e5 - 'arch: changed JSON canonical format, APIs for serialization to LaTeX and MathJSON' (2024-06-25)

^Wherein there is retraction of passing of 'metadata' to ce.box from within ce.parse
And:

e1f9e19 - 'arch' (2024-09-25)

^In which 'metadata' is retracted both as an argument to 'box' (the function) and its inner forwarding to BoxedExpression constructor calls when handling 'MathJSON object literals'.

As a guess, it appears to me that removing metadata as a parameter from box was apt due to these properties being foreseen as present on MathJSON object literals: if present at all. Looks however that in these changes that the step of checking for this meta elsewhere (i.e. object literals) and forwarding onwards (like before) is absent.
This 'fix' is only an educated guess & seems to address the issue,

But this change/request also appears to break a single test-case within test/compute-engine/latex-syntax/numbers.test.ts - which I have not investigated - and thus is not suitable for merging.
(Note at the time of this request, the HEAD of main (#115272d, there is already extant a single test failure (test/compute-engine/functions.test.ts -> Apply -> Function and Hold), which is not related to this change).

Single commit also includes an unused utility isExpressionObject, which was initially used, but could now be discarded.

(Could also do with a couple of test cases for this feature)

@arnog
Copy link
Member

arnog commented Oct 25, 2024

On first reading, this looks like a good PR. Curious to see which test case breaks because of it.

Previously:

```
ce.parse("\text{string}", {preserveLatex: true})
//-> Error(ErrorCode(invalid-identifier, invalid-first-char), "\text{'string'}")
```

With this fix, output from this previous parse-op. is as expected,
('string')
@samueltlg samueltlg changed the title fix: preservation of (verbatim) latex fix: issues parsing with 'preserveLatex' set to 'true' Oct 30, 2024
samueltlg and others added 2 commits October 30, 2024 12:17
`ce.parse("\text{string}", {preserveLatex: true})` now results in a
boxed-expression with string 'string' in contrast to ''string''
@arnog
Copy link
Member

arnog commented Nov 7, 2024

The PR is still a draft, and therefore cannot be merged. Let me know when it's ready for review.

@samueltlg
Copy link
Author

The PR is still a draft, and therefore cannot be merged. Let me know when it's ready for review.

Apologies; I did think it was already ready to be merged.

I just did some investigation into why the breaking test-case, & I think it appears that everything is otherwise fine.
The reason for this particular matcher failure is because parsing of '1.0' with function parseVal within 'compute-engine/latex-syntax/numbers.test.ts', when parsing this with meta-data (now the default), results in this being a number (ExactNumericValue, to be precise), rather than a 'ce.one' instance. Consequently, through parseVal, the string-form of its numericValue is returned being "1", rather than the expected number-literal 1; so maybe just a little tweak somewhere.

Just marking it as ready, anyway

@samueltlg samueltlg marked this pull request as ready for review November 7, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants