-
Notifications
You must be signed in to change notification settings - Fork 11
Asciimath nowrap overscripts #170
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
Asciimath nowrap overscripts #170
Conversation
03a598e to
82a9882
Compare
|
I forgot to update my local repo. Updated, rebased, and force-pushed. I deleted the old fix from #164 |
jolyonb
left a comment
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.
Looks good to me, assuming that your javascript syntax is correct ;-) Just the one comment.
| var middle = '{:' + eqn.slice(funcStart, funcEnd) + ':}' | ||
| eqn = front + middle + eqn.slice(funcEnd) | ||
| var middle = eqn.slice(funcStart, funcEnd) | ||
| eqn = front + wrapIfNotOverscript(null, middle) + eqn.slice(funcEnd) |
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 think this is unnecessary, as nothing in the set contains parentheses, and this is only called for completed functions, right?
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.
wrapFuncCalls wraps incomplete function calls:
wrapFuncCalls('cat(') // returns '{:cat:}(
wrapVariables('cat(') // returns 'cat('Now, I'm not sure that wrapping incomplete function calls is actually necessary... I think when we encountered the issues with x'/2 and p(x)/2, we adopted "wrap everything" as a philosophy, so I wrapped incomplete function calls, too. (Overscript issues has made me question that philosophy...)
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.
Ah, the point was more that the old code is equivalent to the new code, unless I'm missing something?
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 think that the test I added for wrapFuncCalls describes how we want it to behave for overscript symbols:
The middle two cases for that test require this alteration to wrapFuncCalls.
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.
Ah, I see now. Very good! Thanks for the clarifications. For posterity, I'll add that overline() converts to {:overline():}, which is the desired behavior, and works in AsciiMath.
Resolves #169