Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting an exception on non-zero exit code was good. Exceptions for handling control flow in Python are totally fine, and obvious to the user, and hard to miss, I would keep that. (I did not realize that that's what's happening because I did not dig deep enough in the call hierarchy)
The additional logging is good, so I'd keep that too. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception on non-zero exit code has to be handled with care. There are possibilities that a result file is created (and usefull) even if the exit code is != 0. An exception here would prevent any processing of such data.
If I remeber correctly, an example is if the model is terminated via Modelica code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean via
terminate()? If yes a nonzero exit code would be strange, because this "successfully terminates the analysis which was carried out".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, this change is fine. It’s also consistent with how we handle this in OMEdit: we show the user what happened when running the executable, and if there’s any useful output, we use it for post-processing. If an exception is raised, that post-processing will not happen anyway.
Regarding
check=True, it does the following:The new code already covers this behavior: it explicitly checks the
returncodeand logs it.