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

Change doc strings to start with uppercase letter and fix spellings of Units.mo #3470

Merged
merged 17 commits into from
Mar 5, 2020

Conversation

christiankral
Copy link
Contributor

@christiankral christiankral commented Mar 1, 2020

This change is not applied to Units.mo where lots of doc string start with a lower case letter.

Examples:

  • metre
  • revolution
  • angular
  • radian
  • cubic
  • bar
  • second
  • minute

are spelled with a lower case letter at the beginning of the doc string.

@MartinOtter I wonder if we shall switch to all upper case letter at the beginning of each doc string.

@christiankral christiankral added the documentation Issue addresses the documentation label Mar 1, 2020
@christiankral christiankral added this to the MSL4.0.0 milestone Mar 1, 2020
Copy link
Member

@beutlich beutlich left a comment

Choose a reason for hiding this comment

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

How did you do it?

@christiankral
Copy link
Contributor Author

christiankral commented Mar 2, 2020

How did you do it?

Manually ;-)

Search "a, "b, ...

@beutlich
Copy link
Member

beutlich commented Mar 2, 2020

Thought you have a nice regular expression that can be used as CI job.

@dietmarw
Copy link
Member

dietmarw commented Mar 2, 2020

\[\^\\\]\"\[a-z\] or [^\]"[a-z] depending if it needs to get escaped or not.

@HansOlsson
Copy link
Contributor

This change is not applied to Units.mo where lots of doc string start with a lower case letter.

Examples:

  • metre
  • revolution
  • angular
  • radian
  • cubic
  • bar
  • second
  • minute

are spelled with a lower case letter at the beginning of the doc string.

I would assume that it is because the variable name is an abbreviation of the doc-string:

 function to_kmh "Convert from metre per second to kilometre per hour"
      extends Modelica.SIunits.Icons.Conversion;
      input Velocity ms "metre per second value";
      output NonSIunits.Velocity_kmh kmh "kilometre per hour value";

An alternative is "Value in metre per second", "Value in kilometre per hour".

@MartinOtter I wonder if we shall switch to all upper case letter at the beginning of each doc string.

@beutlich
Copy link
Member

beutlich commented Mar 2, 2020

\[\^\\\]\"\[a-z\] or [^\]"[a-z] depending if it needs to get escaped or not.

Too simple. [^\\=\,+]\s+"[a-z] avoids some false matches, but still gives 800 hits in MSL.

@christiankral
Copy link
Contributor Author

I would assume that it is because the variable name is an abbreviation of the doc-string:

 function to_kmh "Convert from metre per second to kilometre per hour"
      extends Modelica.SIunits.Icons.Conversion;
      input Velocity ms "metre per second value";
      output NonSIunits.Velocity_kmh kmh "kilometre per hour value";

An alternative is "Value in metre per second", "Value in kilometre per hour".

I think is a good proposal. I will incorporate it in this PR.

@christiankral
Copy link
Contributor Author

christiankral commented Mar 2, 2020

I applied the following changes:

@HansOlsson
Copy link
Contributor

The changes seem good, but the original reference for the spelling is https://www.bipm.org/utils/common/pdf/si-brochure/SI-Brochure-9-EN.pdf
which in 5.3 for unit names states:

In English, the names of units start with a lower-case letter (even when the symbol for the unit
begins with a capital letter), except at the beginning of a sentence or in capitalized material
such as a title. In keeping with this rule, the correct spelling of the name of the unit with the
symbol °C is “degree Celsius” (the unit degree begins with a lower-case d and the modifier
Celsius begins with an upper-case C because it is a proper name).

@christiankral
Copy link
Contributor Author

christiankral commented Mar 2, 2020

The following changes are applied:

@christiankral christiankral changed the title Change doc strings to start with uppercase letter Change doc strings to start with uppercase letter and fix spellings of Units.mo Mar 2, 2020
@christiankral
Copy link
Contributor Author

I think I am done with the changes in Units.mo. I updated the title of this PR to fully match the applied changes

@christiankral
Copy link
Contributor Author

@beutlich and @dietmarw please re-review Units.mo (all other files are untouched since you carried out your reviews)

Modelica/Units.mo Outdated Show resolved Hide resolved
Comment on lines +1540 to +1541
input Modelica.Units.NonSI.Time_minute minute "Value in minute";
output SI.Time s "Value in second";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
input Modelica.Units.NonSI.Time_minute minute "Value in minute";
output SI.Time s "Value in second";
input Modelica.Units.NonSI.Time_minute minute "Value in minute";
output SI.Time s "Value in second";

Should it be "Value in seconds" and "Value in minutes"? If yes, when to apply the plural?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it be "Value in seconds" and "Value in minutes"? If yes, when to apply the plural?

I was asking this myself many times when applying all these changes. I thought that the plural form in general sounds more correct than the singular form, but I was reluctant to change it, as it causes a massive change...

One way to overcome this issue were to write "Value in unit second" instead of "Value in second". @beutlich what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I am in favour of keeping the singular form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean we keep "Value in second" and do not change it to "Value in unit second"?

Copy link
Member

Choose a reason for hiding this comment

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

Exactly.

@dietmarw
Copy link
Member

dietmarw commented Mar 3, 2020

@christiankral Are you aware that you will have to redo either this PR after #3480 has been merged or the other way round. I guess the latter is less of a job.

@beutlich
Copy link
Member

beutlich commented Mar 3, 2020

Can we merge this?

@HansOlsson
Copy link
Contributor

@christiankral Are you aware that you will have to redo either this PR after #3480 has been merged or the other way round. I guess the latter is less of a job.

It should be, the mere splitting should be straightforward with Dymola 2020x; and likely other tools as well. I could help if needed. And the rest just seemed to be running two commands to remove some excess stuff.

@christiankral
Copy link
Contributor Author

Let's us merge this PR first and I will re-prepare the splitting of Blocks (similar to #3475)

@dietmarw dietmarw merged commit 7b7f133 into modelica:master Mar 5, 2020
@beutlich beutlich removed the request for review from MartinOtter March 5, 2020 10:34
@beutlich beutlich assigned dietmarw and unassigned beutlich Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issue addresses the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants