Replaced many exit(1)s with exception raising
#1468
Conversation
inclement
force-pushed
the
improve_fault_handling
branch
2 times, most recently
from
8be3e16 to
ccb225a
Compare
|
I was wondering if |
| @@ -58,8 +59,7 @@ def check_python_dependencies(): | |||
| ok = False | |||
|
|
|||
| if not ok: | |||
| print('python-for-android is exiting due to the errors above.') | |||
| exit(1) | |||
| print('python-for-android is exiting due to the errors logged above') | |||
There was a problem hiding this comment.
Should there be an error raised here? Or is this change intended?
There was a problem hiding this comment.
I originally did, but actually this code is run before most of the build process (because it has to run before the imports) so I think it needs a different treatment.
There was a problem hiding this comment.
well but as it is, this seems to be a big change in behavior since the code flow just seems to resume. Is that intended? Maybe the exit should be left in if a runtime error doesn't make sense here? Or will this problem of not ok be evaluated again & properly dealt with later?
If you say it makes sense I'll take your word for it, I haven't looked at the followup code closely. it just seems like a potential trouble source breaking things
There was a problem hiding this comment.
I also added the exit(1) back in with the previous comment, I missed that I'd accidentally removed it before so thanks for pointing it out!
There was a problem hiding this comment.
Ah right, the removing line is gone now. Didn't see that 😆
inclement
force-pushed
the
improve_fault_handling
branch
2 times, most recently
from
03cddbb to
c6deaa8
Compare
|
@AndreMiras Just rebased this to fix merge conflicts, any objections to merging it (if the tests pass)? |
|
Just to barge in, I have been eyeing this for a while and I think it's a really good code smell fix. Haven't commented so far since I haven't had the chance to test if it actually doesn't break stuff, but it looks like a thing really worth improving |
| @@ -58,8 +59,7 @@ def check_python_dependencies(): | |||
| ok = False | |||
|
|
|||
| if not ok: | |||
| print('python-for-android is exiting due to the errors above.') | |||
| exit(1) | |||
| print('python-for-android is exiting due to the errors logged above') | |||
There was a problem hiding this comment.
Ah right, the removing line is gone now. Didn't see that 😆
I also think it's a nice improvement, can't wait for it once the tests are passing indeed. We know the conditional build are now failing #1485 but the rest (e.g. linting) should be fixed before we can merge. |
|
Hi @inclement can you give me write access to your |
AndreMiras
force-pushed
the
improve_fault_handling
branch
from
c6deaa8 to
6ed21b4
Compare
AndreMiras
force-pushed
the
improve_fault_handling
branch
from
6ed21b4 to
bec18e1
Compare
This both makes the code neater and will make it easier to test things.