Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Report error if --python-script associated with non-existent file #1018

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Sep 7, 2018

This commit ensures an exception is raised when the python script does not
exist. When associated with the --exit-after-startup option, a non-zero status
code is now returned when Slicer exit.

This commit ensures an exception is raised when the python script does not
exist. When associated with the --exit-after-startup option, a non-zero status
code is now returned when Slicer exit.
@jcfr
Copy link
Member Author

jcfr commented Sep 7, 2018

Cc: @lassoan @ihnorton

Copy link
Contributor

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Would it be possible to throw the error from inside this->corePythonManager()->executeFile()?

Execution can go wrong in many ways and it is odd that one particular error is reported with an external mechanism.

At least we should let executeFile decide if there is an error (file does not exist, not readable, etc) instead of just checking outside if the file exists. This would be also more robust, when script is defined by relative path (executeFile might be able to find it, while QFile::exists cannot).

@jcfr
Copy link
Member Author

jcfr commented Sep 8, 2018

Thanks for the review

Would it be possible to throw the error from inside this->corePythonManager()->executeFile()

Definitively, that makes sense.

For reference:

https://github.com/commontk/CTK/blob/6c0eddaaf8b306c5cf553e8421bca7a5640e380a/Libs/Scripting/Python/Core/ctkAbstractPythonManager.cpp#L303-L333

@jcfr jcfr added the wip label Sep 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants