Clean up code & improve test coverage #249

ondras12345 · 2025-04-12T09:05:02Z

This PR tries to clean up the code by:

using f-strings instead of old %-style format strings and .format(),
using pathlib.Path and .as_posix() instead of os.path.* and manually replacing \ with /,
dropping legacy code that was needed for Python2 compatibility,
dropping unreachable code.

There should be no significant changes in behavior (except for the last commit "Fix createCSVData on Windows", which removes an extra newline at the end of the csv file).

It also adds tests, improving OMPython/__init__.py coverage to 73% on Linux.

OMPython/__init__.py

syntron · 2025-04-16T18:43:56Z

@ondras12345 I realy like the work you have done; especially the string formating! I was thinking about this myself but stopped as I was not sure how much of it is needed for Python 2.x compatibility. However, as this is droped, these changes make the code more readable!

ondras12345 · 2025-04-16T18:56:50Z

how much of it is needed for Python 2.x compatibility. However, as this is droped

I didn't even notice the README said the library still supports Python2, and since setup.py said python_requires='>=3.8', I assumed that's the oldest version we need to support and started removing python2 workarounds in #246.

Also as a side note, if support for 3.8 is declared in pyproject.toml, we should probably run tests @ 3.8 and not just 3.10 and 3.12:

OMPython/.github/workflows/Test.yml

Line 15 in bfb1153

python-version: ['3.10', '3.12']

To be fair, I didn't test my changes with Python 3.8. I should go do that tomorrow...

syntron · 2025-04-16T19:30:19Z

To be fair, I didn't test my changes with Python 3.8. I should go do that tomorrow...

I have to save, I did my testing only with Python 3.10+; thus, it would be helpfull to get a feedback that version is the lowest to support before running any tests nobody cares about ...

ondras12345 · 2025-04-17T11:43:41Z

I tried to run a set of tests with python 3.8, and it doesn't even install. And it's totally my fault, because it fails to parse pyproject.toml, which was introduced by me in #246. Error log:
https://github.com/ondras12345/OMPython/actions/runs/14511691030/job/40711663255
I'll see what I can do about it.

ondras12345 · 2025-04-17T12:00:05Z

It seems like it expects a deprecated format:

If you get a build error that license should be a dict/table, your build backend doesn’t yet support the new format. See the above section for more context. The now deprecated format is described in PEP 621.
https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license-and-license-files

The new format is only supported since setuptools 77.0.0, which in turn require python 3.9: https://github.com/pypa/setuptools/blob/v77.0.0/pyproject.toml#L25. If we really want python 3.8 compatibility, we might have to drop the license specifier in pyproject.toml.

adeas31 · 2025-04-17T12:39:23Z

I think we should update to Python version 3.12, it was release 2023-10-02 and has end of life until 2028-10 see https://devguide.python.org/versions/. There is no point in supporting old python versions. It might be that some of the dependencies might not work with python 3.12 but we can try and see.

Let me know once you have finalized the PR and I will try to review and merge it asap.

ondras12345 · 2025-04-17T13:19:07Z

I'm currently working on resolving the conflicts.

I don't think jumping all the way to 3.12 is necessary. E.g. debian 12 ships with python 3.11.2, so there is value in supporting older python version for linux users.

It might be that some of the dependencies might not work with python 3.12

CI tests currently run on 3.10 and 3.12:

OMPython/.github/workflows/Test.yml

Line 15 in 630229a

python-version: ['3.10', '3.12']

and they are passing. It should be safe to say python >= 3.10 is supported. It's not like we need the new features introduced in 3.11 and 3.12.

- use f-strings to make sendExpression() arguments easier to read, - use pathlib.Path to convert \ to / in file paths on Windows - fix a few bugs (mostly missing spaces in error strings)

It behaves differently before and after simulate()

I'm pretty sure this also fixes a bug: the only Exception I can see os.path.join() raise in _get_omc_path() is when self.omhome is None, which shouldn't really happen (the constructor checks it). I think the intention was to check whether the file actually exists, so I do that instead.

... instead of raw sendExpression. Seems cleaner and improves test coverage.

... by dropping Python2 support

... except for logging calls. It probably doesn't matter, but the built-in %-formatting does not run when logging is disabled, improving performance.

... probably due to missing .exe suffix.

The csv module documentation says to open the file with newline='': https://docs.python.org/3/library/csv.html#id4 Also, I'm pretty sure this isn't how you are supposed to use csv.writer. delimiter='\n' doesn't seem right. We should either let the csv writer actually generate the rows, or remove it altogether and just write the string rows into the file.

syntron · 2025-04-17T14:19:03Z

and they are passing. It should be safe to say python >= 3.10 is supported. It's not like we need the new features introduced in 3.11 and 3.12.

I support staying with Python >= 3.10 as this version is working well in the tests; why should we limit the possible user range without a really need for such a change?

ondras12345 · 2025-04-17T14:24:38Z

I have rebased my branch onto master and fixed the conflicts (there was a lot of them).
If the tests pass (especially the windows ones, I already ran linux tests locally), this PR should be ready for review @adeas31

As discussed in OpenModelica#249

syntron · 2025-04-17T15:05:58Z

I have rebased my branch onto master and fixed the conflicts (there was a lot of them). If the tests pass (especially the windows ones, I already ran linux tests locally), this PR should be ready for review @adeas31

Sorry for the work ... I will try to keep the next changes generic such that these can be easily rebased based on your PR

arun3688

looks good

adeas31

LGTM. Thanks!

As discussed in #249 Co-authored-by: Adeel Asghar <[email protected]>

syntron mentioned this pull request Apr 15, 2025

drop Python 2.x in README.md #255

Merged

ondras12345 mentioned this pull request Apr 16, 2025

[ModelicaSystem._run_cmd] use argument cwd of subprocess.Popen() inst… #251

Closed

syntron reviewed Apr 16, 2025

View reviewed changes

OMPython/__init__.py Outdated Show resolved Hide resolved

adeas31 requested review from adeas31 and arun3688 April 17, 2025 12:34

adeas31 mentioned this pull request Apr 17, 2025

next release? #254

Closed

ondras12345 added 17 commits April 17, 2025 15:21

Fix testModelicaSystemLoop

c199951

Use python3 features to clean up ModelicaSystem

831a7d8

- use f-strings to make sendExpression() arguments easier to read, - use pathlib.Path to convert \ to / in file paths on Windows - fix a few bugs (mostly missing spaces in error strings)

Fix warning in test_linearization

0fe21a5

Add more tests for ModelicaSystem methods

f5d07e6

Add test for ModelicaSystem.getLinear*()

1304f8d

Add test for ModelicaSystem.getContinuous()

aab0957

It behaves differently before and after simulate()

Improve test coverage

0369afd

Remove unreachable code

da9eb45

Add test for ModelicaSystem.optimize()

04714e6

Add tests for simulate() edge cases

a18a6b3

Improve test coverage

1ce9ab8

Use setCommandLineOptions()

ba03555

... instead of raw sendExpression. Seems cleaner and improves test coverage.

Use f-strings in OMCSessionZMQ

7f4e0a9

Simplify omc_process kill logic

50b2898

... by dropping Python2 support

Drop Python2 workarounds

1246d47

Use f-strings in OMCSessionZMQ

f67d393

... except for logging calls. It probably doesn't matter, but the built-in %-formatting does not run when logging is disabled, improving performance.

ondras12345 added 5 commits April 17, 2025 16:07

Remove unreachable code

1b94486

Fix _get_omc_path fails on Windows

02728a8

... probably due to missing .exe suffix.

Do not run omc with shell=True

747cc68

Remove last remaining str.format() call

2be6fbd

ondras12345 force-pushed the refactor-f-strings branch from 20c24dc to 2be6fbd Compare April 17, 2025 14:18

ondras12345 added a commit to ondras12345/OMPython that referenced this pull request Apr 17, 2025

Set requires-python to >=3.10

d3bb72c

As discussed in OpenModelica#249

ondras12345 mentioned this pull request Apr 17, 2025

Set requires-python to >=3.10 #256

Merged

arun3688 approved these changes Apr 17, 2025

View reviewed changes

ondras12345 mentioned this pull request Apr 17, 2025

[ModelicaSystem] do NOT store data in the class workspace #252

Merged

syntron mentioned this pull request Apr 17, 2025

Refactor file structure #257

Merged

adeas31 approved these changes Apr 23, 2025

View reviewed changes

adeas31 merged commit d63e2d8 into OpenModelica:master Apr 23, 2025
5 checks passed

adeas31 added a commit that referenced this pull request Apr 23, 2025

Set requires-python to >=3.10 (#256)

8df1ab8

As discussed in #249 Co-authored-by: Adeel Asghar <[email protected]>

ondras12345 mentioned this pull request Apr 23, 2025

Replace print() with logger.warning #262

Merged

Clean up code & improve test coverage #249

Clean up code & improve test coverage #249

Uh oh!

Conversation

ondras12345 commented Apr 12, 2025

Uh oh!

Uh oh!

syntron commented Apr 16, 2025

Uh oh!

ondras12345 commented Apr 16, 2025

Uh oh!

syntron commented Apr 16, 2025

Uh oh!

ondras12345 commented Apr 17, 2025

Uh oh!

ondras12345 commented Apr 17, 2025

Uh oh!

adeas31 commented Apr 17, 2025

Uh oh!

ondras12345 commented Apr 17, 2025

Uh oh!

syntron commented Apr 17, 2025

Uh oh!

ondras12345 commented Apr 17, 2025

Uh oh!

syntron commented Apr 17, 2025

Uh oh!

arun3688 left a comment

Choose a reason for hiding this comment

Uh oh!

adeas31 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants