-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add comments with TODOs after LaTeXML issue is fixed #2882
Merged
HansOlsson
merged 4 commits into
modelica:master
from
henrikt-ma:comment-mathescape-emph
Mar 5, 2021
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
31528c1
Add comments with TODOs after LaTeXML issue is fixed
henrikt-ma cc49ebd
Work around LaTeXML/MathJax issue with \emph in mathescape
henrikt-ma c114714
Expect MathJax to become obsolete rather than workaround in LaTeXML
henrikt-ma 633b9d8
Say "colon" before (:) (that is, a colon inside parentheses)
henrikt-ma File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm not sure about skipping angles here (and the similar line below). The n here should normally be replaced by a number (such as 3 for quaternions).
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.
Couldn't we solve this nicely by rephrasing this slightly and placing it before the listing?
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 also like having it as
<n>
.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 don't find the angle bracket construct very pretty in general, but still useful for placeholders in need of a textual description. For something acting as a placeholder for a number, I find it more consistent to use the rather well established use of "meta variables" typeset as math.
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.
What are the well-established meta-variables? Where are they established?
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.
It strikes me now that the use of n in the listing would actually assume that it is an
Integer
dimension – is this really the current intention? To me, the current text would actually allow an array indexed byBoolean
or enumeration (both of which will result in a constant size, which is the requirement), and I don't see that it would really be a problem to do so. If we want to make it more clear that this is allowed, I suggest this:In the text, we could elaborate this further:
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 would be OK with removing the angled brackets around n. But I feel like we need to open a separate issue to clarify what "known size" means. If we come up with a concise wording we could consider putting that into the angle brackets in the code.
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.
Created #2886.
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.
Good. @HansOlsson, would you then be OK with the resolution to move (with some rephrasing) the sentence introducing n to before the listing?
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 would be ok with different variants of that - and I agree with @eshmoylova that it seems best to have a separate issue for that, so resolving, accepting and merging this.