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

Add recommendation for code formatting (and apply it) #4221

Merged
merged 22 commits into from Jan 11, 2024

Conversation

HansOlsson
Copy link
Contributor

Add some description of how we want code to be formatted, and apply the rules.
Some minor items to clean up.

@HansOlsson HansOlsson marked this pull request as ready for review November 3, 2023 10:07
@tobolar
Copy link
Contributor

tobolar commented Nov 6, 2023

@HansOlsson I miss a notice how to indentate multi-line declarations.
And, a code example could be helpfull as well.

@HansOlsson
Copy link
Contributor Author

@HansOlsson I miss a notice how to indentate multi-line declarations. And, a code example could be helpfull as well.

I tried to add both of them.

Copy link
Contributor

@tobolar tobolar left a comment

Choose a reason for hiding this comment

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

I'm in favor with this suggestion.
Even if we could go farther similar to Python's PEP8 - Indentation it would be probably too restrictive for now.

@tobolar tobolar added documentation Issue addresses the documentation L: UsersGuide Issue addresses Modelica.UsersGuide labels Nov 7, 2023
@tobolar tobolar changed the title Formatting Add recommendation for code formatting (and apply it) Nov 7, 2023
@HansOlsson
Copy link
Contributor Author

@casella as new MAP-Lib leader I hope you approve of this.

@HansOlsson
Copy link
Contributor Author

@casella @henrikt-ma can you review it - so that it can be merged before release-branch on Friday January 12th?

I believe that the previous formatting in Digitial is due to someone having
protected A
protected B
protected C
(which would be another style).
Then the subsequent protected were lost, and first protected formatted on a separate line.
(Additional changes would be good.)
@HansOlsson
Copy link
Contributor Author

I have now rebased it on master and removed three new cases of trailing white-space. It would be good if this could be approved and merged, @casella @henrikt-ma

Copy link
Contributor

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

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

The conventions look sensible, and I can't spot anything obviously wrong with the included changes.

What I'd like to avoid however, is the risk of styling conventions for the Modelica Standard Library to be taken as conventions that should apply to all Modelica code. To avoid this, I suggest changing this sentence:

<p>A Modelica main package should be compliant with the UsersGuide stated in this documentation:</p>

Something like this would make more sense to me:

<p>A Modelica main package of the Modelica Standard Library should be compliant with the UsersGuide stated in this documentation:</p>

By the way, I had never heard of a Modelica main package until I saw this line – is it a generally recognized term? Does it mean the same as what I'd refer to as an MSL sub-library?

@henrikt-ma
Copy link
Contributor

I have now rebased it on master and removed three new cases of trailing white-space. It would be good if this could be approved and merged, @casella @henrikt-ma

Please avoid rebasing, as it breaks GitHub conversations linked to commits. For example, I was unable to find my previous review comments in order to see if and how they had been addressed.

@HansOlsson
Copy link
Contributor Author

I have now rebased it on master and removed three new cases of trailing white-space. It would be good if this could be approved and merged, @casella @henrikt-ma

Please avoid rebasing, as it breaks GitHub conversations linked to commits. For example, I was unable to find my previous review comments in order to see if and how they had been addressed.

Rebasing is unfortunately needed to ensure easier merging - as the status was too messy, but that's why it should only be done after addressing the comments.

@HansOlsson
Copy link
Contributor Author

The conventions look sensible, and I can't spot anything obviously wrong with the included changes.

What I'd like to avoid however, is the risk of styling conventions for the Modelica Standard Library to be taken as conventions that should apply to all Modelica code. To avoid this, I suggest changing this sentence:

<p>A Modelica main package should be compliant with the UsersGuide stated in this documentation:</p>

Something like this would make more sense to me:

<p>A Modelica main package of the Modelica Standard Library should be compliant with the UsersGuide stated in this documentation:</p>

By the way, I had never heard of a Modelica main package until I saw this line – is it a generally recognized term? Does it mean the same as what I'd refer to as an MSL sub-library?

That line wasn't changed; and the UsersGuide already contained recommendations that could get into that problem.

So, once more: don't block PRs based on other existing issues

Copy link
Contributor

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

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

OK, the potential risk of these guidelines being misinterpreted as applying to all Modelica code (not just in the MSL) isn't new and will be reported separately.

@HansOlsson HansOlsson merged commit 62cf230 into modelica:master Jan 11, 2024
2 checks passed
@HansOlsson HansOlsson deleted the Formatting branch January 11, 2024 10:30
@beutlich beutlich added this to the MSL4.1.0 milestone Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issue addresses the documentation L: UsersGuide Issue addresses Modelica.UsersGuide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants