Miscellaneous enhancement suggestions

[modelonrobinandersson]

Hi,

I have been looking through the code a bit in import_fmu_to_modelica.py and I have a couple of suggestions in terms of enhancements. Feel free to disagree to these since these are just my personal opinions:

1. Currently on line 49 we have makedirs(unzipdir, exist_ok=True), but if anything fails after this part there still will be files created from the failed import. It would be good to wrap this in a try/finally in order to clean-up anything was generated before the failure.
2. I find the name of the second argument model_path a bit confusing, i.e. def import_fmu_to_modelica(fmu_path, model_path, interface_type):. Would it make more sense if this was named something in line with library_path or package_path? I.e. a user chooses the FMU, and the Modelica library to import to. And there could be a default value for the generated mo-file, where the default name is based on the FMU.
3. There does not seem to be any error handling for long paths on Windows environments. Note that the directories created on line 49 with makedirs can cause issues on Windows since there is a maximum path limit. This can be accounted for in Python but even if that is done, a user might have issues when trying to remove/edit these directories (if you create an extremely long path in Windows and try to delete it, it usually doesn't work).
4. There is a lot of hardcoded / in the code, I would suggest constructing paths solely using os.path.join(a,b, ...). It might not matter but I wouldn't be surprised if there is some odd corner case where '/' fails and os.path.join is considered a safer approach.
5. In the generated mo-file it would be good if it can include some metadata as description/annotation documentation, for example: Model generated by modelica-fmi version ... .
6. Parameters such as communicationStepSize, tolerance and others (parameters added by the import and not from the FMU) could be annotated or named such that it is clear to a user that they come from the package itself and not from the imported FMU. I remember this was already the case in FMPy for example parameter Real tolerance = 0.0 annotation(Dialog(tab="FMI", group="Parameters")); but perhaps those could be clarified even more?

[t-sommer]

1. Currently on line 49 we have makedirs(unzipdir, exist_ok=True), but if anything fails after this part there still will be files created from the failed import. It would be good to wrap this in a try/finally in order to clean-up anything was generated before the failure.

We're not just creating but also changing files. I think the better strategy is to save before the import and revert if it fails.

3. There does not seem to be any error handling for long paths on Windows environments.

This is under the control of the user.

see my comment in #8

5. & 6.

Please open individual issue and or PRs.

[modelonrobinandersson]

6. annotation(Dialog(tab="FMI", group="Parameters"))

Thank you Torsten! I see you opened #10 already, I created #11 just now.
Regarding point 1, I agree that could also be an option. I have no strong opinion besides that files are not left on the file system, i.e. the user should have the same state after the failure as the user had before the failure.
Before we close this issue however, could you comment on point 4, how is this under the control of the user? My experience is that this usually ends up with strange error messages and very often the user does not even know there is a path length limit on Windows machines.

[t-sommer]

how is this under the control of the user?

The users should enable long paths (https://learn.microsoft.com/en-us/answers/questions/1805411/how-to-enable-long-file-path-names-in-windows-11). I don't see how we can handle this in the import.

[modelonrobinandersson]

@t-sommer I think with path issues on Windows, most users will not understand the underlying issue but in the module one can check path lengths before attempting to create/move any file and give an appropriate error.