- Notifications
You must be signed in to change notification settings - Fork51
fix: various metadata preservation issues (boxing, LaTeX-parsing)#212
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
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')`ce.parse("\text{string}", {preserveLatex: true})` now results in aboxed-expression with string 'string' in contrast to ''string''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. Just marking it as ready, anyway |
-Consequently, as would be expected, allows usage of engine-scopedcommon symbols (ce.One, et cetera) in some scenarios (instead ofneedlessly missing out), fixing some test cases predicated upon thisalong the way
-*should result in correct preservation of metadata (latex,wikidata) insome scenarios: such as number boxing (ce.number), and some cases offunction boxing ('box', 'boxFunction') - wherever explicitly given.Good that this has not been merged yet! - detected a couple more related issues. |
| latex?:string|undefined; | ||
| wikidata?:string|undefined; | ||
| }; | ||
| exporttypeMetadata=Pick<Attributes,'latex'|'wikidata'>; |
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.
May wish to revert this: personally just thought it made sense.
| _fn( | ||
| name:MathJsonIdentifier, | ||
| ops:ReadonlyArray<BoxedExpression>, | ||
| options?:Metadata&{canonical?:boolean} |
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.
Quite sure that that was a mistake, or at least non-desirable
Uh oh!
There was an error while loading.Please reload this page.
Noticed that
verbatimLatexnot being saved on BoxedExpression instances created fromce.parsefor some time. Looks as if this traceable to commits:^Wherein there is retraction of passing of 'metadata' to
ce.boxfrom withince.parseAnd:
^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
metadataas a parameter fromboxwas 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 tobreak 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)