Exit with a status code if Fire encounters an error#27
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Copybara generated commit for Python Fire. PiperOrigin-RevId: 150818132 Change-Id: Ice3b2977fe0e89fa6472f0831e7475f3318943cf Reviewed-on: https://team-review.git.corp.google.com/65029 Reviewed-by: David Bieber <[email protected]>
fire/__init__.py Outdated
| fromfire.coreimportFireExit | ||
| __all__= ['Fire'] | ||
| __all__= ['Fire', 'FireExit'] |
There was a problem hiding this comment.
Perhaps don't expose FireExit yet until the API for FireTrace stabilizes.
In the mean time, the tests can assertRaises a SystemExit rather than a FireExit.
There was a problem hiding this comment.
I think this is a fair point and agree with it (tho maybe nice to be able to say "exit status of 1 means client exception, exit status of 2 means bad parameter or fire invocation") - I went ahead in a PR and added a helper to check output from FireExit anyways.
| raiseFireExit(1, component_trace) | ||
| elifcomponent_trace.show_traceandcomponent_trace.show_help: | ||
| print('Fire trace:\n{trace}\n'.format(trace=component_trace)) | ||
| result=component_trace.GetResult() |
There was a problem hiding this comment.
Revisit lines 136+139+144.
| These exceptions are not raised by the Fire function, but rather are caught | ||
| and added to the FireTrace. | ||
| """ |
There was a problem hiding this comment.
Should FireError be private?
They are stored in the FireTraces, which are now accessible outside of Fire via the FireExit.
…ss (#28) * FireExit passing tests, ape argparse, doc update 1. Update documentation slightly 2. Get all tests to pass and note some additional edge cases 3. add helper method to capture all stdout and make it easier to write regexps for comparison 4. On proper `--help`, do the same thing as argparse and exit 0. * Always include component trace + explicitly check type on exception * Lint fixes and doc changes * Move testutils to separate file * Mock argv rather than pass in empty command * Lint fixes * Switch to raising exit code of 0 on show trace and show help * Do not shadow component trace
googlebot commented Mar 22, 2017
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
dbieber commented Mar 22, 2017
Oh, silly awkward googlebot. |
jtratner commented Mar 22, 2017
Yes I'm okay with it :) (and signed a CLA already) |
fire/fire_test.py Outdated
| self.assertIsNone(fire.Fire(tc.NoDefaults, 'double')) # Missing argument. | ||
| self.assertIsNone(fire.Fire(tc.TypedProperties, 'delta x')) # Missing key. | ||
| # Exceptions of Fire are printed to stderr and a FireExit is raised. | ||
| withself.assertRaises(fire.FireExit): |
There was a problem hiding this comment.
Going to switch to assertRaisesFireExit for all of these.
Copybara generated commit for Python Fire. PiperOrigin-RevId: 150942368 Change-Id: I7ba73d5a9845d642f99ffeba6c953d8116830a33 Reviewed-on: https://team-review.git.corp.google.com/65058 Reviewed-by: David Bieber <[email protected]>
dbieber commented Mar 22, 2017
All set! Thanks for the contribution! |
jtratner commented Mar 23, 2017
Yeah - happy to help out! Really cool project that I'm excited to dig into further! Slight bummer that the squashing ended up eliminating my contribution completely from the history but I guess that's life. |
dbieber commented Mar 23, 2017
Yeah, was just looking at that :/ |
jtratner commented Mar 23, 2017
Yeah I think it would. |
Checked in by David Bieber, authored by David and Jeff. * Exit with a status code if Fire encounters an error * adds FireExit and assertRaisesFireExit * does not expose FireExit publicly yet
dbieber commented Mar 23, 2017
Done. Hope I don't set off any alarms checking something in as you :P. |
jtratner commented Mar 23, 2017
Thanks - I appreciate the consideration :) |
dbieber commented Mar 23, 2017
And I appreciate the contribution 👍 |
3 participants
Exit with a status code if Fire encounters an error
Introduces the "FireExit" exception, a subclass of SystemExit.