Skip to content

Update modelica system #277

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

Merged
merged 16 commits into from
May 6, 2025
Merged

Conversation

syntron
Copy link
Contributor

@syntron syntron commented Apr 30, 2025

Update ModelicaSystem on top of PR #273

Changes:

  • remove verbose - use logger.debug()
  • remove raiseerror - use raise ModelicaSystemException()
  • use pathlib.Path() if possible
  • enforce lmodel is a list
  • use exceptions if possible

@syntron
Copy link
Contributor Author

syntron commented Apr 30, 2025

This needs PR #272 - removed this dependency and solved the dependency problem by a temporary solution - use Exception instead of OMCSessionException for now ...

@syntron syntron force-pushed the update_ModelicaSystem branch from 784f937 to 34921ee Compare April 30, 2025 18:24
This was referenced May 1, 2025
@syntron syntron force-pushed the update_ModelicaSystem branch from 2c3ab36 to 92d804a Compare May 3, 2025 19:34
@adeas31
Copy link
Member

adeas31 commented May 5, 2025

Seems this one is next in line. Please rebase it so I can start reviewing it.

@syntron syntron force-pushed the update_ModelicaSystem branch from 92d804a to 1433499 Compare May 5, 2025 19:42
@syntron
Copy link
Contributor Author

syntron commented May 5, 2025

Seems this one is next in line. Please rebase it so I can start reviewing it.

@adeas31 done

Copy link
Member

@adeas31 adeas31 left a comment

Choose a reason for hiding this comment

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

LGTM.

However, the verbose flag calls getErrorString() which is useful in some cases to show the compiler notifications to the user. For example, if you call this,

loadString("model M annotation (uses(Modelica(version=\"4.0.0\"))); end M;");

You will not get the notifications that the Modelica package is loaded due to uses annotation unless you call ``getErrorString()`.

I suggest we always call getErrorString() inside sendExpression and logs it using the logger. To further enhance this, we can use getMessagesStringInternal() instead of getErrorString() since getErrorString() returns string without any structure you don't know if its an error, warning or a notification. If we use getMessagesStringInternal() we can properly log using error, warning and info.

@syntron
Copy link
Contributor Author

syntron commented May 6, 2025

I suggest we always call getErrorString() inside sendExpression and logs it using the logger. To further enhance this, we can use getMessagesStringInternal() instead of getErrorString() since getErrorString() returns string without any structure you don't know if its an error, warning or a notification. If we use getMessagesStringInternal() we can properly log using error, warning and info.

This is done in PR #278; please check mainly 7658886; I will check the usage of getMessagesStringInternal()

@adeas31 adeas31 merged commit 00f894c into OpenModelica:master May 6, 2025
5 checks passed
@syntron syntron deleted the update_ModelicaSystem branch May 8, 2025 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants