Skip to content

[OMCSession*] Cleanup / mypy check - #291

Merged
adeas31 merged 12 commits into
OpenModelica:masterfrom
syntron:cleanup_OMCSessionZMQ
Jun 5, 2025
Merged

[OMCSession*] Cleanup / mypy check#291
adeas31 merged 12 commits into
OpenModelica:masterfrom
syntron:cleanup_OMCSessionZMQ

Conversation

@syntron

Copy link
Copy Markdown
Contributor

fix mypy warnings in OMCSession and some additional cleanup

@syntronsyntron changed the title Cleanup omc session zmq[OMCSession*] Cleanup / mypy checkMay 28, 2025
This was referenced May 28, 2025
self._dockerOpenModelicaPath = dockerOpenModelicaPath
self._dockerNetwork = dockerNetwork
self._create_omc_log_file("port")
self._omc_log_file = self._create_omc_log_file("port")

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.

Isn't the best practice is generally to set the value of a member variable directly within a member function?

@syntronsyntronJun 2, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is a good question - I'm not sure; my reasoning is as follows but if there are different views I'm fine with it - but this would mean to rewrite this PR for some parts ...

  • an internal / private function is used (starting with '_')
  • within these class functions all class members can be accessed (but should not be changed)
  • I tried to put the definition of class members via return value and set the value in init() as it is quite visible this way
  • furthermore, it is required to set / define class variables in init()
  • at the end, these helper functions are just cutouts from init() to make it better readable

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.

  • an internal / private function is used (starting with '_')

That is fine.

  • within these class functions all class members can be accessed (but should not be changed)
  • I tried to put the definition of class members via return value and set the value in init() as it is quite visible this way
  • furthermore, it is required to set / define class variables in init()
  • at the end, these helper functions are just cutouts from init() to make it better readable

You can't enforce this programmatically. It is still possible to access the member with a dot notation. It is like we are expecting the current/future developers will follow these guidelines.

I think the pythonic way to handle this is to use the python property https://docs.python.org/3/library/functions.html#property. If we do this, the users/developers can still access the class member with dot notation but a getter/setter is always called so we know from where the value of a member is set or accessed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I thing using property would be the way to go if there is a need to enforce this. Here, it is more a way to make the code more readable - class variables which are set are directly visible in __init__() with the source of the used value.

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.

OK. I am fine with it.

@syntron
syntronforce-pushed the cleanup_OMCSessionZMQ branch from a4a0d75 to cf937ccCompareJune 4, 2025 19:08
@adeas31
adeas31 merged commit c57b8bc into OpenModelica:masterJun 5, 2025
@syntron
syntron deleted the cleanup_OMCSessionZMQ branch June 5, 2025 19:39
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.

2 participants

@syntron@adeas31