Skip to content
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

import Modelica.SIunits #2960

Closed
AHaumer opened this issue Jun 6, 2019 · 8 comments
Closed

import Modelica.SIunits #2960

AHaumer opened this issue Jun 6, 2019 · 8 comments
Assignees
Labels
discussion Discussion issue that it not necessarily related to a concrete bug or feature P: high High priority issue
Milestone

Comments

@AHaumer
Copy link
Contributor

AHaumer commented Jun 6, 2019

In my opinion, the usage of import SI=Modelica.SIunits; at some place in a sublibrary to shorten parameter definitions is really bad style:

  • hard to read: Where is the import statement located?
  • If you copy a model out of the MSL, the import gets broken.
    Much better style would be: import Modelica.SIunits.*; in every model.
  • easy to read: The import statement is in the model I'm looking at.
  • If you copy a model out of the MSL, the import goes with the model, nothing gets broken.

The question is:
Should we go through the whole MSL and change the style (resp. location) of such import statements?

@AHaumer AHaumer added enhancement New feature or enhancement P: high High priority issue labels Jun 6, 2019
@AHaumer AHaumer added this to the MSL4.0.0 milestone Jun 6, 2019
@christiankral
Copy link
Contributor

I think this is a very good idea! I am in favor of improving the quality of the Modelica Standard Library that way!

@AHaumer AHaumer added discussion Discussion issue that it not necessarily related to a concrete bug or feature question Unexplained or undecided issue labels Jun 6, 2019
@AHaumer
Copy link
Contributor Author

AHaumer commented Jun 6, 2019

Compare 3 styles:

  1. parameter Modelica.SIunits.Mass m=100 "Total mass";
    Clear but not elegant.
  2. parameter SI.Mass m=100 "Totoal mass";
    import SI=Modelica.SIunits elsewhere
    Neither Clear nor elegant.
  3. parameter Mass m=100 "Totoal mass";
    import Modelica.SIunits.* just above
    Clear and elegant.

@dietmarw
Copy link
Member

dietmarw commented Jun 7, 2019

Sorry, but NEVER EVER use wildcard imports. We even write in the specs to not use them as they are dangerous since there is a huge danger of variable collision. So for imports only ever use implicit imports. They (i.e., wildcard imports) are simply bad programming style in any language.
As for import SI=Modelica.SIunits this is something that is down to taste. I use this, and like this, extensively together with import C=Modelica.Contstants. So definitely don't start changing this in the MSL. I find it much easier on the eye to not have the whole Modelica.SIunits. prefix all over the place.

@AHaumer
Copy link
Contributor Author

AHaumer commented Jun 7, 2019

What about copy a model out of the MSL for applying own changes?

@HansOlsson
Copy link
Contributor

What about copy a model out of the MSL for applying own changes?

I see three possible solutions:

  • Use a tool that automatically handles this when copying.
  • Always use full path, e.g., Modelica.SIunits.Mass
  • Have import SI=Modelica.SIunits; in every model. I find that solution tedious.

@dietmarw
Copy link
Member

dietmarw commented Jun 11, 2019

The solution would definitely be on the tool side in this case. And Dymola does this already by recognising the imported definitions and replacing them by full qualified class names upon duplication. So it really is a non-issue. And if it is then it is a missing tool feature. I know that OpenModelica for example doesn't even recognise relative name lookup on duplication at the moment. But they are working on it and the recognition of imports would just be another such feature that needs then to be worked on by the tool (@adrpo FYI).

@AHaumer
Copy link
Contributor Author

AHaumer commented Jun 12, 2019

ok you're right

@AHaumer AHaumer closed this as completed Jun 12, 2019
@beutlich beutlich removed the question Unexplained or undecided issue label Jun 12, 2019
@beutlich
Copy link
Member

beutlich commented Jun 12, 2019

Compare 3 styles:

1. `parameter Modelica.SIunits.Mass m=100 "Total mass";`
   Clear but not elegant.

2. `parameter SI.Mass m=100 "Totoal mass";`
   `import SI=Modelica.SIunits` elsewhere
   Neither Clear nor elegant.

3. `parameter Mass m=100 "Totoal mass";`
   `import Modelica.SIunits.*` just above
   Clear and elegant.

The recommendation is to use unnamed import statements, i.e.,

parameter SIunits.Mass m=100 "Total mass";
import Modelica.SIunits;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion issue that it not necessarily related to a concrete bug or feature P: high High priority issue
Projects
None yet
Development

No branches or pull requests

5 participants