-
Notifications
You must be signed in to change notification settings - Fork 65
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
✍️ LaTeX/JATS improvements for Proofs #694
Conversation
packages/myst-spec-ext/src/types.ts
Outdated
/** | ||
* Line is, e.g., a line in an algorithm and can be numbered as well as indented. | ||
* Otherwise this works the same as a paragraph, ideally with tighter styling. | ||
* The Line is used in Algorithms (e.g. when parsing from LaTeX) | ||
*/ | ||
export type Line = Parent & { type: 'line'; indent?: number; enumerator?: string }; |
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.
@fwkoch this is probably the most controversial change in the PR. I have added a new node of type Line
which can have indentation and line numbers. This is to support the algorithm parsing, falling back to a div here is totally fine in the current theme.
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.
Cool, my only thoughts are:
- The name
Line
conflicts withline
in unist positions; maybe slightly confusing that it does not correspond to a "line" in the source. But that's probably ok? Not sure what else we'd call it,Step
? Meh - something more specific likeAgorithmLine
? (This would be similar toDefinitionTerm
which can be used in places other than just definitions.) - Why not just add
indent
andenumerator
as optional attributes onParagraph
? We overload most of the other default node types with extra attributes. That would make fall-back behaviour more straightforward (just ignore the attributes)... I dunno, maybe then it feels like we are going down a path of "you can enumerate everything" which feels... complicated (as opposed to having intentionality about what may be enumerated).
Overall, though, this is probably fine?
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 did start there, adding attributes to paragraph, and went back and forth. I wanted the JAST to have a "specific-use" identifier and the UI to have a special style, and this seemed special enough for its own type. A div is also probably a more appropriate fallback based on style than a paragraph in this case?
At some point we should do an assessment of these types and do a clean up. I think that we can collapse a few things like admonitionTitle
and proof
--> statement
perhaps.
I changed this to algorithmLine
based on your first point!
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 skimmed over a lot of the tex macro, etc, additions (looks like there is decent test coverage, but it doesn't quite get it all - not a big deal, tons of repetition...).
Overall, looks good, left my comments on Line
but fine with me to go ahead here.
packages/myst-spec-ext/src/types.ts
Outdated
/** | ||
* Line is, e.g., a line in an algorithm and can be numbered as well as indented. | ||
* Otherwise this works the same as a paragraph, ideally with tighter styling. | ||
* The Line is used in Algorithms (e.g. when parsing from LaTeX) | ||
*/ | ||
export type Line = Parent & { type: 'line'; indent?: number; enumerator?: string }; |
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.
Cool, my only thoughts are:
- The name
Line
conflicts withline
in unist positions; maybe slightly confusing that it does not correspond to a "line" in the source. But that's probably ok? Not sure what else we'd call it,Step
? Meh - something more specific likeAgorithmLine
? (This would be similar toDefinitionTerm
which can be used in places other than just definitions.) - Why not just add
indent
andenumerator
as optional attributes onParagraph
? We overload most of the other default node types with extra attributes. That would make fall-back behaviour more straightforward (just ignore the attributes)... I dunno, maybe then it feels like we are going down a path of "you can enumerate everything" which feels... complicated (as opposed to having intentionality about what may be enumerated).
Overall, though, this is probably fine?
paragraphs.forEach((p, i) => { | ||
const l = p as unknown as Line; | ||
l.type = 'line'; | ||
l.enumerator = String(i + 1); |
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.
This enumeration is at a granular level, without the customization allowed for other enumerators. Again, probably fine but it means the enumerator
attribute works in different ways.
Maybe, though, having all the normal enumerator customization here is also the long-term goal.
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 did start out with this being called "number", but also felt like we should use common terminology. Certainly a bit different than the other enumerators, with this one being much more granular.
const label = texToText(labelNode); | ||
const countWith = texToText(x) || undefined; | ||
const countAfter = texToText(y) || undefined; | ||
state.data.theorems[name] = { label, countWith, countAfter }; |
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 feel like I'm missing where these theorems stored on data
are actually consumed... 🤔 Maybe they just aren't yet...?
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.
They are not yet, correct. When we start enumerating theorems in a custom way, they this count information will likely be used.
Thanks for the review @fwkoch, I am going to move ahead with this being a separate type, having this be different enough from a paragraph in the latex and jats implementations. Not totally happy with it, but also I think this is something we can back out of in the future when/if we get to a better solution. |
This parses algorithm environments in latex and adds the serializers for JATS. There are also a number of latex parsing improvements.