-
Notifications
You must be signed in to change notification settings - Fork 40
Apply modern table presentation to some more annotations #2957
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
Apply modern table presentation to some more annotations #2957
Conversation
With the merge conflicts now resolved, maybe this would be a good time to get this merged? |
@HansOlsson don't you think merging this could be considered a nice step in the direction for annotation cleanup decided at the last web meeting? |
Possibly, but I saw that only 1 of 2 tasks were completed, and thus skipped it. |
|
||
The \lstinline!ModelicaLibraryName! used below for \lstinline!IncludeDirectory!, \lstinline!LibraryDirectory!, and \lstinline!SourceDirectory! indicates the top-level class where the annotation is found in the Modelica source code. | ||
|
||
\begin{annotationdefinition}[Library] |
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 prefer if we just write that it should be a string or an array of strings, and don't use grammar fragments unless needed.
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.
Are you sure this isn't a good place to use it? It is defined once, used in the definitions of 5 different annotations so far, and also mentioned in the running text. It would take considerably more text and hand waving to do it without giving a name to the idea.
While we might be able to change many of the annotations to the pseudo-code style of presentation, the ones we have here belong to the category where specification using grammar actually makes sense as we can't express the type of the annotation using a Modelica type.
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 just keep the grammar fragment for now, and revisit this once we take the bigger grip on presentation of annotations as decided at the last phone meeting (can't find in what issue that decision was noted)?
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.
Any chance we can return to this once we turn to the question of how to present the things that can't be expressed easily in the record style established so far?
@HansOlsson, would it help moving this forward if I request reviews from other people? I feel we are making progress on the presentation on so many annotations now, and it would be nice if we could move on to the principal discussion about what to do with things like the deeply nested pseudo-code record structures and the unstructured annotations that can't be presented as pseudo-code records. |
Merge conflicts have been resolved, so this is once again ready. Adding another reviewer, hoping for more action. |
Possibly, but:
|
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 looked through the changes. But before I start excessively commenting on individual items, I'd like to clarify what the goal of this change. Do we want to update all instances of annotation definitions? What are we updated them to? I see 3 styles:
- Pseudo-record definition
- Grammar style
documentation-class-annotation :
annotation "(" DocumentationClass "=" true ")"
- Piece of code, e.g.
annotation(preferredView = view)
I see some of the style 3 converted into style 2 by this PR but not all. Is that ok or do we want to find all instances?
Also, it seems to me that the decision was to convert more to the record style. Of course, that would apply only to structured annotations. For consistency, it would make sense to present simple annotations as component declarations:
String defaultComponentName = "name";
But it is harder with cases where only couple of values are allowed. For example, preferredView can only be "info", "diagram", "icon" and "text". You would need to say something like:
String preferredView = view;
where view is one of "info", diagram", "icon", and "text".
To me the record style is preferable.
Or we could use another annotation to describe that:
I understand it looks close to recursive, but it might still be useful. |
I thought about using choices. What I am not sure about is: does it indicate that no other values are allowed? The description of choices only says that each choice indicates a suitable redeclaration or modification. But are those suitable values the only ones that are allowed? Maybe it does't matter. Probably if the preferredView is something other that the ones above it would be ignored. Or is it supposed to give an error? |
I would say it doesn't matter much. I could imagine that we later want to extend this (similarly as "icon" was added); so we shouldn't make it a hard error. |
My intention with this PR is only to modernize the presentation of some more annotations, not to fix the global problem of getting all annotations into the new form, whatever that is. However, after this PR, I think it would be the right time to take a step back and think about what styles we should really use in the end. Some questions to think about are mentioned in the initial comment of this PR. (With the recent introduction of the section on notation in the introduction, we now have a perfect place for explaining how the different variants are to be interpreted, and we should reference these explanations from the beginning of the Annotations chapter.) |
Didn't we just spend quite a lot of time to come up with the current design where the variable view is used in the "structured presentation" of the annotation, with a description of the allowed values for view given in the text below? I don't think it's that bad, and at least less strange compared to explaining annotations in terms of |
To me that seems odd. I would first define what style we prefer as the modern one (as stated above I prefer the "record-style" regardless of whether it is records, scalars, arrays or pseudo-records), and then try to work to get all (or most) annotations to use that. It might also be that this PR is trying to do two things (which is always complicated):
And we focus on different parts of this, you might be looking at the first part and the rest of us are stuck on the second part. |
@henrikt-ma, @HansOlsson, @sjoelund, I would like to view the build for this PR, so that I don't have to build it myself. I used to be able to access the automatic builds on https://test.openmodelica.org/jenkins/job/ModelicaAssociation/job/ModelicaSpecification/view/change-requests/ Now when I click on the link it asks sign in and no option to sign up or forgot password or anything. Am I allowed to get access to that and, if yes, how? |
@sjoelund have you changed anything? |
While waiting for CI builds to become available again, I'll send the document directly to @eshmoylova. |
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.
The addition of tables looks good. As already discussed, I don't really like that you had to convert some annotation definitions to the grammar style but I see how it was necessary in some cases to introduce some formal definition to fit with the rest of the style. So it's not easy to just get back to what it was without losing the tables. I would be ok with keeping the grammar style annotations as presented in this PR for the time being just so that we can move on with adding the tables and then, as discussed, opening a new issue and new PR for unifying the format of annotations. Perhaps it would be better to rename this PR into "Modernize table presentation of some more annotation"?
One more thought about the format of type/record vs grammar before I forget it: how would you represent the choice annotation in a record format? It is supposed to be any possible modification on the class for which it is specified, so the structure of the annotation is the same as the corresponding class.
I did a small variation of that. |
I don't know. Some annotations will be easier than others to express without the use of grammar. I think |
Looking more I don't think that using textvisiblespace is significantly better than just plain space. |
The title of the PR looks good. But I wonder if the description should be updated a bit as well. Maybe just adding updating "modernizing the presentation of annotations" to "modernizing the presentation of annotations using tables" in the first paragraph? |
Sure, no problem. Done. |
…tring Addressing review comment by Elena.
Co-authored-by: Elena Shmoylova <[email protected]>
Would it be meaningful to resolve the merge conflicts at this point, or should I keep waiting? |
I think you can resolve them. |
9f89d36
to
6033119
Compare
Merge conflicts have been resolved, and at least there's one approval from @eshmoylova now. Maybe we could merge and then turn our attention to the more challenging questions about how to present annotations? |
The general idea with tables seem good.
|
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.
The general idea with tables seem good.
But:
- There are too many changes unrelated to the main idea. A PR shall be more focused. In this case there have been so many unrelated changes that it is difficult to see what the main idea is. That's why the PR has been stuck - it is not possible to see what the actual idea is.
- Changing to using grammar for annotations reduces readability. It would be understandable for annotations that weren't described well previously, but here it is also applied to e.g. versionDate which was a good example of using a non-record declaration. Using a grammar to describe that is just a step backwards.
chapters/inheritance.tex
Outdated
@@ -983,60 +983,107 @@ \subsection{Restrictions on Redeclarations}\label{restrictions-on-redeclarations | |||
\end{nonnormative} | |||
\end{itemize} | |||
|
|||
\subsection{Annotation Choices for Suggested Redeclarations and Modifications}\label{annotation-choices-for-suggested-redeclarations-and-modifications} | |||
\subsection{Annotations for Suggested Redeclarations and Modifications}\label{annotations-for-suggested-redeclarations-and-modifications} |
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.
By keeping the previous label (in addition to the new ones) I believe that we keep external references to the HTML.
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 checked it, and only the last \label
is turned into the id
of a section
tag in the HTML. Hence, we should only have one \label
, and it would be good if it was more stable than the exact formulation of the section title.
|
||
It can also be applied to nonreplaceable declarations, e.g.\ to describe enumerations. |
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 line seems lost in the new version.
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 is there, but formulated as a stand-alone introduction to a separate example:
Using \lstinline!choices! to suggest suitable value modifications for a non-replaceable declaration:
@@ -178,7 +178,7 @@ \subsection{Main changes in Modelica 3.5}\label{main-changes-in-modelica-3-5} | |||
Ticket \href{https://github.com/modelica/ModelicaSpecification/issues/2288}{\#2288}. | |||
\item Added test-case annotation for incorrect models, \cref{modelica:TestCase}. | |||
Ticket \href{https://github.com/modelica/ModelicaSpecification/issues/2340}{\#2340}. | |||
\item Clarified choicesAllMatching, \cref{annotation-choices-for-suggested-redeclarations-and-modifications}. | |||
\item Clarified \lstinline!choicesAllMatching!, \cref{annotations-for-suggested-redeclarations-and-modifications}. |
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.
The previous proposal of keeping the old label would mean that we could keep the revisions-file intact which seems preferable.
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.
Never making edits to revisions.tex isn't possible with a document that gets reorganized over time. I also don't see the value of un-touched LaTeX source code.
As an alternative to updating cross references from the revision history, one could consider replacing obsolete cross references with a standard formulation such as (original context of change is no longer available), instead of updating the cross references to point to the current best available approximation of the original reference.
@@ -437,15 +437,31 @@ \section{Annotations for Simulations}\label{annotations-for-simulations} | |||
|
|||
\section{Annotation for Single Use of Class}\label{annotation-for-single-use-of-class} | |||
|
|||
For state machines it is useful to have single instances of local classes. |
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.
Explaining the purpose of single instance at the start is important; so I don't like removing it.
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 is still there:
This is useful for local classes in the state machines described in \cref{state-machines}.
The annotation \fmtannotationindex{singleInstance} in a class indicates that there should only be one component instance of the class, and it should be in the same scope as the class is defined. | ||
\begin{annotationdefinition}[singleInstance] | ||
\begin{synopsis}[grammar]\begin{lstlisting} | ||
"singleInstance" "=" true |
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.
Looking at this example, I don't think that using grammar-syntax for annotations is an improvement in most cases.
Obviously it is needed in a few cases, but in most cases the previous examples and/or defining it using "pseudo-variables" (such as Boolean singleInstance=false;
would be cleaner.
The problems I see with grammar-rules:
- The quoting makes it harder to see the actual rule.
- I don't think anyone is going to implement it as a grammar-rule.
- It makes it harder to think about using parameter-values instead of literals (obviously not in this case).
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.
Neither do I expect the average tool to implement this based on the grammar rules, but the formalism has the advantage of greater precision. For example, it is straight-forward to express that the annotation expects a literal value, and not a general parameter expression that might need messy evaluation of parameters etc.
Still, I am ready to switch to something such as pseudo-variable as soon as we have worked out a systematic way of making use of them. The use of grammar should therefore only be considered a temporary fix using the best formalism available for this kind of annotations today.
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.
But using the grammar it is harder to specify that it expects a parameter expression of a specific type, whereas we have a push from users to make more annotations support non-literal values.
And the quoting is still distracting, and the PR is still too broad.
annotation( | ||
defaultComponentName = "world", | ||
defaultComponentPrefixes = "inner replaceable", | ||
missingInnerMessage = "The World object is missing" | ||
); |
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's not that this change is bad, it's just that it is not related to the main theme of using tables, and thus it's difficult to see the main 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.
I could factor it out, but the problem is that starting another PR from the branch of this PR easily gets confusing in the Github workflow. I see the problem with scope creep, but I sometimes find it better to do it this way than dealing with a potentially large number of tiny PRs that would be full of merge conflicts after merging the main PR (or, even worse, cause a large number of merge conflicts for the main PR).
"All Forces cannot be uniquely calculated. The reason could be that the " + | ||
"mechanism contains a planar loop or that joints constrain the same motion.\n" + | ||
"For planar loops, use in one revolute joint per loop the option " + | ||
"PlanarCutJoint = true in the Advanced menu." | ||
); |
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's not that this change is bad, it's just that it is not related to the main theme of using tables, and thus it's difficult to see the main 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.
Same here. The good thing is that everything is under the umbrella of cleaning up the presentation of annotations. This PR is free of semantical changes that would require the attention of the bigger language group.
@@ -1407,7 +1425,7 @@ \subsection{Connector Sizing}\label{connector-sizing} | |||
|
|||
block Step | |||
// nIn cannot be set through the dialog (but maybe shown) | |||
parameter Integer nIn=0 annotation(Dialog(connectorSizing=true)); | |||
parameter Integer nIn = 0 annotation(Dialog(connectorSizing = true)); |
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's not that this change is bad, it's just that it is not related to the main theme of using tables, and thus it's difficult to see the main 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.
Sorry, same reasons as above.
Addressing comment by Hans.
Due to request by Hans.
As far as I can see:
I thus think it is best to close this one, and if needed open a new one for the stylistic changes from scratch, and if possible add more "table presentation". |
What started with the intention of just fixing a minor formatting issue related to
missingInngerMessage
ended up as a wider sweep of modernizing the presentation of annotations using tables. With this, all annotations presented outside the Annotations chapter use the modern style. In particular, all annotations in the Functions chapter are now presented in the same way.Ironically,
missingInngerMessage
still belongs to one of the annotation sections where the presentation hasn't been modernized. It is one of the two more obvious sections next in turn for modernization (possibly added to this PR to avoid merge conflicts, unless this PR is closed quickly):Then there are also the annotations presented using more or less deeply nested structures of pseudo code records, and these are not as clear how to modernize. Examples include: