-
Notifications
You must be signed in to change notification settings - Fork 164
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
Rename Modelica.Icons.{RotationalSensor, TranslationalSensor} #3016
Conversation
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.
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 stumble a bit about the naming of the icons. As I stated in #2301 (comment), I rather prefer to follow the Indexing rules decribed e.g. in https://www.asindexing.org/best-indexing-practices. Following that rules, a subheading (round, rectangular) shall be predeceased by a main heading (sensor). The suggested names follow indeed reverse order.
I know there is no such rule defined in MSL, but even in the Modelica.Icons
package itself, there can be found both approaches - simply compare names of "package" icons to those of "type" icons.
So my recommendation is to use SensorRectangular
instead of RectangularSensor
(and similarly for RoundSensor
.
As I know this could trigger a large discussion (which is fine), I only leave my concern as a comment.
|
||
partial class TranslationalSensor | ||
partial class RectangularSensor | ||
"Icon representing a linear measurement device" |
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 element's desription (and documentation) shall be modified accordingly. Whereby the RoundSensor
is "Icon representing a round measurement device", the RectangularSensor
is "... a linear measurement device".
To be really honest, is is not clear at all when to use which icon for a sensor component. Neither it is described in Modelica.UsersGuide.Conventions.Icons
.
A link to Modelica.UsersGuide.Conventions.Icons
shall be established in the documentation of the two sensor icons as well.
@@ -931,7 +931,7 @@ Conversion test for <a href=\"https://github.com/modelica/ModelicaStandardLibrar | |||
|
|||
package Icons | |||
extends Modelica.Icons.ExamplesPackage; | |||
model Issue813 "Conversion test for #813" | |||
model Issue340 "Conversion test for #340" |
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.
Does not this concern both 813 and 340 issues?
Some examples: * = current implementation
|
Is there a membership required to access the PDF file provided by the link above? I get the following error message when trying to access the PDF file: |
The new notation would require to rename most of the actual class names. So someone familiar with the structure of the MSL, will not recognize it any more after the re-design. So I am not convinced that this is the right way to go. However, even though I could live with most of the new names, the class name
In the very same way, for example, sources could be structured as:
I think it were difficult to decide when a new package shall be introduced. Are two machine models enough? So the structuring would also change the existing hierarchy of the MSL a lot. To me it seems to be a an overkill, too. |
No idea why the link is not working. Please find the pdf via https://www.asindexing.org/best-indexing-practices. |
This would definitely be the consequence. But I agree it would require too much effort. So let us just make a notice of this. |
Please stop this discussion ... I'm strictly agianst such crazy things like |
@AHaumer This PR does not introduce any crazy namings. This PR sticks to the existing naming conventions. So you can just go ahead regularly review this PR. |
Closes #340.