-
Notifications
You must be signed in to change notification settings - Fork 43
Generalize modern table presentation with application to Protection #3770
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
base: master
Are you sure you want to change the base?
Generalize modern table presentation with application to Protection #3770
Conversation
|
|
||
| \begin{annotationdefinition*}[access]\label{modelica:Protection.access}\annotationindex{access}\index{Protection@\robustinline{Protection}!\robustinline{access} sub-annotation} | ||
| \begin{synopsis}\begin{lstlisting} | ||
| /*literal*/ constant Access Protection.access; |
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.
| /*literal*/ constant Access Protection.access; | |
| /*literal*/ constant Access access; |
Making it consistent with the other ones.
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.
You are right that the PR is currently inconsistent and that this needs to be fixed.
However, I am not sure it's such a good idea to present the sub-annotations in the same way as the annotations residing directly under annotation(…).
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.
Well, many sub-annotations are defined as part of a record https://specification.modelica.org/master/annotations.html#annotations-for-documentation https://specification.modelica.org/master/annotations.html#annotations-for-figures https://specification.modelica.org/master/annotations.html#modelica:Dialog
So, the alternative would be to have "record Protection" preceding this declaration as in the current specification, but it would seem odd with just one element, and normally the entry would then be called "Protection".
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 see some similarities with those examples, but as you already noted they are not quite the same:
- The
Documentationis not a sub-annotation, so it could be converted to the new style if we really wanted to. However, there is not that much to say aboutDocumentationas a whole, so it makes sense to me that we don't present it in the same way as an annotation which we are able to cover in more detail without only referring to other sections. - The
Figureis a type and not the name of an annotation, so it looks correct to me that we don't present it as an annotation by itself, and that we don't include it in any summary table. However, if there was a way to presentDocumentation.figuresas a sub-annotation, it would have been nice to do so at the top of https://specification.modelica.org/master/annotations.html#annotations-for-figures, something like this:
Figure[:] Documentation.figures;
record Figure
/*literal*/ constant String title = "" "Title meant for display";
/*literal*/ constant String identifier = "" "Identifier meant for programmatic access";
/*literal*/ constant String group = "" "Name of figure group";
/*literal*/ constant Boolean preferred = false "Automatically display figure after simulation";
Plot[:] plots "Plots";
/*literal*/ constant String caption = "" "Figure caption";
end Figure;
- I find the
Dialogproperly presented already. WithloadSelectorand friends being described together with the rest ofDialog, I don't see a need to change anything regarding them. (Having separate annotation definitions forDialog.loadSelectoretc would just look odd to me since they are such an integral part ofDialog.)
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.
Well, one thing to consider is that not only is it different, it also breaks the idea that the annotations are described by legal Modelica code.
E.g., we use /*literal*/ to avoid having a special keyword, whereas Protection.access is not a valid name for a component - and the most obvious way of handling it: 'Protection.access' would just create more confusion.
So, having record Protection in front of may seem a bit excessive, but at least we have some precedence and it is legal Modelica.
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 problems I see with record Protection are:
- It no longer fits nicely on a line.
- We need to indicate that this is just one out of several members of the record.
I mean, this becomes a very cumbersome way to say that the access member a Protection is a /*literal*/ constant Access:
record Protection
/*literal*/ constant Access access;
$\ldots$
end Protection;
While I would still much prefer the non-valid Modelica Protection.access notation, one could also consider some sort of class modification notation:
Protection(/*literal*/ constant Access access);
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.
Well, the text already has a link to the rest of the contents of Protection, and master already has the
record Protection
/*literal*/ constant Access access;
$\ldots$
end Protection;
So, why not keep it for now?
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.
Various variants were considered, including having Protection in the text and having the full Protection-annotation followed by simplified form.
Open a new issue for resolving this.
This PR aims to make the presentation of the
Protectionannotation more similar to the presentation of other annotations, even thoughProtectiononly acts as a container for sub-annotations which are defined in more detail one by one. If we can work this out forProtection, it should be straightforward to treatDocumentationin the same manner.