fix: various metadata preservation issues (boxing, LaTeX-parsing) #212
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')
samueltlg
changed the title
`ce.parse("\text{string}", {preserveLatex: true})` now results in a
boxed-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-scoped common symbols (ce.One, et cetera) in some scenarios (instead of needlessly missing out), fixing some test cases predicated upon this along the way
-*should result in correct preservation of metadata (latex,wikidata) in
some scenarios: such as number boxing (ce.number), and some cases of
function boxing ('box', 'boxFunction') - wherever explicitly given.
|
Good that this has not been merged yet! - detected a couple more related issues. |
samueltlg
changed the title
| latex?: string | undefined; | ||
| wikidata?: string | undefined; | ||
| }; | ||
| export type Metadata = Pick<Attributes, 'latex' | 'wikidata'>; |
There was a problem hiding this comment.
May wish to revert this: personally just thought it made sense.
| @@ -2015,10 +2015,13 @@ export class ComputeEngine implements IComputeEngine { | |||
| _fn( | |||
| name: MathJsonIdentifier, | |||
| ops: ReadonlyArray<BoxedExpression>, | |||
| options?: Metadata & { canonical?: boolean } | |||
There was a problem hiding this comment.
Quite sure that that was a mistake, or at least non-desirable
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 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)