Skip to content

Conversation

@atrosinenko
Copy link

To start with, add an experiment annotation as an example, so one can debug this feature and extend it separately.

This PR is related to OpenModelica/OMEdit#219 and is based on already existing text in this repo, as well as a Modelica Standard. Unfortunately, this will probably be partial copy-paste of ModelicaReference.Annotations instead of plain its usage, since descriptions there are more standard-related and human-readable than machine-readable formal record-like form.

Another idea, is to gather there documentation on the keywords, too, to not hardcode it in the IDE code.

@atrosinenko
Copy link
Author

default

package AutoCompletion "Auto completion information for Modelica IDEs"
package Annotations "Auto completion information on annotations"
record experiment "Define default experiment parameters"
Real StartTime(unit = "s") "Default start time of simulation";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of hardcoding the unit here it should be using the proper type Modelica.SIunits.Time instead of Real. Same for the rest.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little bit weird actually. Because ModelicaReference does not depend on the standard library. So I think it's correct to define it as Real with a unit set manually...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Didn't think about that.

Real Tolerance(min = 0) "Default relative integration tolerance";
end experiment;
annotation(
Documentation(info = "<html><head></head><body>In this package annotations are gathered in their record-like form together with meta information such as descriptions, units, min, max, etc.</body></html>"));
Copy link
Member

@dietmarw dietmarw Feb 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove superfluous <head> and <body> tags.

@atrosinenko atrosinenko force-pushed the auto-completion-reference branch 2 times, most recently from f74f5c3 to c7020c1 Compare February 25, 2019 09:19
@atrosinenko
Copy link
Author

Fixed tags, restored units.

@HansOlsson
Copy link
Contributor

I'm not sure if this fits into ModelicaReference at the moment (there is some old MCP(s) about standardizing annotations as Modelica code - which has been inactive for quite some time).

The leads to the second part: if it weren't in ModelicaReference - but somewhere else - the unit-dependency would also be a non-issue.

@dietmarw
Copy link
Member

dietmarw commented Feb 25, 2019

I think the ModelicaReference is a good place for all existing, standardised, annotations. But I would not want them duplicated. So if the existing documentation could be combined with the type then I think it would be a nice enhancement.

I can not see a problem in changing the existing

class experiment "experiment"
  extends ModelicaReference.Icons.Information;
  annotation ();
end experiment;

to

record experiment "experiment"
  extends ModelicaReference.Icons.Information;
  Real StartTime(unit = "s") "Default start time of simulation";
  Real StopTime(unit = "s") "Default stop time of simulation";
  Real Interval(unit = "s", min = 0) "Resolution for the result grid";
  Real Tolerance(unit = "1", min = 0) "Default relative integration tolerance";
  annotation ();
end experiment;

@atrosinenko
Copy link
Author

Agree, duplicating is bad, I thought it is inherently user-readable docs, but I have not noticed it can be naturally combined.

record experiment "experiment"
                   ^ -- should this always be equal to the record name or can be changed?

@atrosinenko
Copy link
Author

Hmm, I realized why description == name -- it is viewed better in doc browser then. Is it necessary to have some short summaries at all (in some "strict" machine readable form, that is, not "the first paragraph if it is HTML, otherwise the first string, or maybe something else...") or always show the full HTML description (it would probably be useful for the user).

@dietmarw
Copy link
Member

Could you rephrase that? I'm not sure I understand what you mean.

@HansOlsson
Copy link
Contributor

I think the ModelicaReference is a good place for all existing, standardised, annotations. But I would not want them duplicated.

I agree that duplication within ModelicaReference would be bad. It is possible that ModelicaReference is a good place for the annotations in this form, but I'm not certain. My issue is that we will try to do two things with one class and that it may create conflicting requirements; not only for this class - but also e.g., for marking which classes are relevant in this way. Maybe that is not a problem in practice - I don't know.

@atrosinenko
Copy link
Author

@dietmarw I tried first to ask whether I need record recordName "recordName" instead of traditional record recordName "Record description for user", but then I opened the second variant in a documentation browser of Open Modelica and seen 1) duplication of the summary (copied from the first paragraph) 2) lack of easily noticeable annotation name -- so I need to use this record name "name" form.

default

@atrosinenko
Copy link
Author

@HansOlsson my original thoughts were:

  1. I had to implement annotation autocompletion
  2. I implemented it in the editor, so I need some place to fetch the annotation descriptions from
    • hardcode it in C++: reimplementing description machinery for hierarchical structures, types of fields, their units, etc. In addition, it would be maintained by the editor developers while the compiler developers have more info
    • implement it as a Modelica package: this would reuse parsers and storage format, allows more easy editing, even HTML snippet generator from regular autocompletion. Even more, AFAIK it is the record form that considered kind of reference form for annotation description
  3. Then @casella suggested to move it to standard library for general use

Some random thoughts:

  • Other autocompletion can be fetched likewise: keywords, etc. (so the original structure: ModelicaReference / AutoCompletion / Annotations / experiment)
  • It would be great to have vendor annotations here as well -- it may be handy for understanding / porting purposes

@atrosinenko
Copy link
Author

Another thought: maybe user-accessible Modelica package would encourage more people to contribute to this machine-readable documentation (as well as will be easily browsable source of "textbook-form" annotation descriptions) but another separate root package may clutter package tree in editor.

@dietmarw
Copy link
Member

In order to move forward. Any problem to add the annotations as I suggested?
As for vendor annotations, those should be kept inside the tool. We (MAP-LIB) can not take care of them. So only standardised annotation will be added to ModelicaReference.

@dietmarw dietmarw requested a review from HansOlsson February 25, 2019 17:26
@dietmarw dietmarw self-assigned this Feb 25, 2019
To start with, add an experiment annotation as an example.
@atrosinenko atrosinenko force-pushed the auto-completion-reference branch from c7020c1 to 432058a Compare February 25, 2019 17:28
@atrosinenko
Copy link
Author

Refactored as suggested. I just waited a bit to collect different opinions. For now, refactored only single annotation, so I can support this and see how it would behave.

Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically the PR contains correct information.

But I'm unsure if we want it. On one hand it is a good idea to have annotations defined using code - making them clearer.

On the other hand it is not clear if it will solve the original issue (auto-completion in OMEdit), since vendor-annotations are missing and I assume they are useful for this (e.g. to set solver method); and we also have the conflicting idea of having a special "documentation class" replacing "class" and now suddenly experiment is a "record".

Looking through the rest of ModelicaReference.Annotations I notice that only this one, Dialog, and Documentation could have contents defined in this way. (And some that are missing.)

However, I also noticed that absoluteValue, choicesAllMatching, dateModified, defaultComponentName, defaultComponentPrefixes, defaultConnectionStructurallyInconsistent, DocumentationClass, Evaluate, HideResult, InlineAfterIndexReduction, LateInline, ... are all attributes that could be defined using primitive types, similarly as the contents of this annotation. (There are some more complicated ones like choices, derivative, DynamicSelect, and Inverse.)

I don't know how to define e.g. "Boolean absoluteValue;" as an annotation for components in a good way - or even if we should, but I believe we should consider that before adding this. Obviously obsolete and preferredView require special care - since the class defining the obsolete-annotation isn't obsolete itself.

@tobolar tobolar added the L: ModelicaReference Issue addresses ModelicaReference label Feb 26, 2019
@thorade
Copy link
Contributor

thorade commented Feb 26, 2019

There is one technology that I would like to promote whenever I hear auto-completion or syntax-highlight and it is the Language Server Protocol (LSP)!

LSP was inventend and open-sourced by Microsoft for Visual Studio Code for offering advanced IDE features (as the mentioned ones, but also go to definition, type checking, linting, code folding, find usage, prettyprint etc). LSP is a protocol for communication between an editor/IDE and a language server. On the editor side it is supported by Visual Studio Code, emacs, Sublime, Atom, Eclipse and many others. On the language side, there are servers for most of the popular languages (C++, Python, Go, Rust...).

But, there is no language server yet for Modelica! Most of the features required like parsing a library are implemented in each tool, Dymola, OpenModelica, JModelica..., so in my naive view, implementing a Modelica language server would be a somewhat low hanging fruit!?

Read more about LSP here:
https://microsoft.github.io/language-server-protocol/
https://langserver.org/

See some nice animated gifs of LSP in action here:
https://github.com/palantir/python-language-server

@atrosinenko
Copy link
Author

But, there is no language server yet for Modelica!

I even tried to implement it in Scala with OMC as a backend (at the first glance, it seemed the simplest solution to me), but stumbled upon LSP support in different editors (or probably my implementation was broken, but diagnostic in tools was quite poor -- most probably I was doing something wrong...). If this does matter, it was about half-a-year ago.

@dietmarw
Copy link
Member

dietmarw commented Feb 26, 2019

Based on the feedback until now it really turns into a tool specific problem. And unless a complete solution based on Modelica syntax is presented and can be evaluated as replacing (parts) of ModelicaReference I can only agree with @HansOlsson that ModelicaReference is not the right place (yet).

@atrosinenko I suggest you continue with AutoCompletion library inside the OpenModelica/OMEdit repository. Then we can still see if it makes sense to move this into here. As mentioned, there have been previous attempts made to "classify" the Annotations but this seems to be non-trivial.

@dietmarw dietmarw added this to the never milestone Feb 26, 2019
@dietmarw dietmarw closed this Feb 26, 2019
@casella
Copy link
Contributor

casella commented Feb 26, 2019

Based on the feedback until now it really turns into a tool specific problem. And unless a complete solution based on Modelica syntax is presented and can be evaluated as replacing (parts) of ModelicaReference I can only agree with @HansOlsson that ModelicaReference is not the right place (yet).

@atrosinenko I suggest you continue with AutoCompletion library inside the OpenModelica/OMEdit repository. Then we can still see of it makes sense to move this into here.

Sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L: ModelicaReference Issue addresses ModelicaReference wontfix Issue that will not be fixed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants