-
Notifications
You must be signed in to change notification settings - Fork 165
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
Remove unused code (#3622) #3623
Conversation
Removing protected parameters (even if unused) seems problematic with current versioning rules. |
Indeed. It is not covered by conversion rules. For that reason, we mentioned every removed component as backward-compatibility breaking change in the release notes and require a new major version. |
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.
Remove obsolete code is OK
So we could remove the protected parameters from functions, as these quantities cannot be accessed from outside and keep them in models and blocks. Additionally, all commented code fragments shall be removed. |
See adrpo#1 |
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'm against removing the unused code.
It is written as comment and makes the code easier to understand.
So I'm against this PR.
@AHaumer @adrpo Would you agree to better document the code lines? I understand that @AHaumer wants to keep the commented code as it represents an alternative (inverse) representation of the used equations. Example: Change Real RotationMatrix[2, 2]={{+cos(-angle),-sin(-angle)},{+sin(-angle),+
cos(-angle)}};
//Real InverseRotator[2,2] = {{+cos(+angle),-sin(+angle)},{+sin(+angle),+cos(+angle)}};
...
//u = InverseRotator*y; to Real RotationMatrix[2, 2]={{+cos(-angle),-sin(-angle)},{+sin(-angle),+
cos(-angle)}};
// Alternative equivalent implementation:
// Real InverseRotator[2,2] = {{+cos(+angle),-sin(+angle)},{+sin(+angle),+cos(+angle)}};
...
// Alternative equivalent implementation:
// u = InverseRotator*y;
|
Is OK from my side, I thought it was some left over code, but if this is documentation, of course we should keep it. |
I'd prefer to keep the comments as internal documentation - thanks. |
Add alternative implementation comment and revert comment removal for those implementation
Add alternative implementation comment and revert comment removal for those implementation
Add alternative implementation comment and revert comment removal for those implementation
Add alternative implementation comment and revert comment removal for those implementation
Add alternative implementation comment and revert comment removal for those implementation
@AHaumer @christiankral, we have made the |
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.
ok for me
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.
OK with me.
There are some components defined that are not used in the blocks or functions.
This PR removes them.