Skip to content

Refactor omc session classes - #250

Merged
adeas31 merged 3 commits into
OpenModelica:masterfrom
syntron:refactor_OMCSession_classes
Apr 17, 2025
Merged

Refactor omc session classes#250
adeas31 merged 3 commits into
OpenModelica:masterfrom
syntron:refactor_OMCSession_classes

Conversation

@syntron

@syntronsyntron commented Apr 15, 2025

Copy link
Copy Markdown
Contributor

merge OMCSessionHelper into OMCSessionZMQ

define execute as deprecated

merge (parts of) OMCSessionBase into OMCSessionZMQ

  • class OMCSessionBase only contains ask() and derived functions to call OM
  • the moved functions were using variables of class OMCSessionZMQ
  • this change splits the class definitions into
    (1) code to connect to OM => OMCSessionBase
    (2) code to execute OM / connect to running OM => OMCSessionZMQ

@syntron

Copy link
Copy Markdown
ContributorAuthor

based on #240 - cleanup of the remaining classes ...

@casella
casella requested a review from arun3688April 15, 2025 20:18
@arun3688

Copy link
Copy Markdown
Collaborator

@syntron can you resolve the conflicts and update the PR

* class OMCSessionBase only contains ask() and derived functions to call OM
* the moved functions were using variables of class OMCSessionZMQ
* this change splits the class definitions into
(1) code to connect to OM => OMCSessionBase
(2) code to execute OM / connect to running OM => OMCSessionZMQ
@syntron
syntronforce-pushed the refactor_OMCSession_classes branch from 8f38d66 to 5bd67daCompareApril 16, 2025 18:27
@syntron

Copy link
Copy Markdown
ContributorAuthor

@syntron can you resolve the conflicts and update the PR

Done

@adeas31adeas31 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

Maybe in another PR we can also remove OMCSessionBase as it doesn't serve any purpose now.

@adeas31
adeas31 merged commit 630229a into OpenModelica:masterApr 17, 2025
@syntron
syntron deleted the refactor_OMCSession_classes branch April 17, 2025 15:46
@syntron

syntron commented Apr 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Maybe in another PR we can also remove OMCSessionBase as it doesn't serve any purpose now.

I could create such a commit; however, my proposal would be to only add some cleanup as the current structure quite nicely splits
(1) OM commands => OMCSessionBase (only needs an implementation of sendExpression()) and
(2) code needed to connect to an OM session => OMCSesssionZMQ

This way, it would be possible to keep OMCSessionZMQ clean. Furthermore, it could allow to split it into code which is needed to run OM locally and the code which is related to docker based execution. I expect, that the docker case would be something which could run on top of the local execution.

@syntronsyntron mentioned this pull request Apr 17, 2025
Sign up for freeto 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.

3 participants

@syntron@arun3688@adeas31