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 |
| 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 |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
haven't tested it 😬 but looks nice. and I think it's a really useful internal improvement
| 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
AndreMiras
left a comment
AndreMiras
left a comment
There was a problem hiding this comment.
Fixed rebase conflict, linter and unit tests, good to go
2 participants
This both makes the code neater and will make it easier to test things.