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

add post-mortem pdb flag #71

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kapilt
Copy link

@kapilt kapilt commented May 21, 2017

this adds an additional fire flag (-- -d|--debug) which provides for automatically dropping a user into post mortem pdb session on exception in the underlying exposed callable.

@dbieber
Copy link
Member

dbieber commented May 21, 2017

I definitely see the value of this feature.
However, I'm hesitant to add a new flag for this.

Here are the reasons for my hesitation:

  1. Keeping the API simple.
    I don't want a proliferation of flags. In fact, I'd like to move in the opposite direction and remove flags: e.g. a) I'd like to remove --completion by having a single completion script that works for all Fire CLIs, and b) possibly we can merge verbose and trace into a single flag.
  2. We're also discussing another use of the keyword "debug" for Fire CLIs. This discussion revolves around making the output of Fire CLIs more palatable to non-developer clients of the CLIs, while still keeping the more developer-centric information (filepaths, line numbers, etc) available. This probably more properly falls under the purview of the --verbose flag. We'll want to sort out precisely what verbose + debug will do moving forward (e.g. introducing multiple levels of verbosity) before introducing this pdb/debug feature.
  3. A third thing about this feature [in it's current state] that I'm hesitant about is that the --debug flag only does something if there's an error. This means that someone can run a command w/ and w/o the --debug flag, trying to figure out what it does, and will be confused thinking it does nothing.

All of that said, this does seem really useful. Perhaps after I try it out for a few days I'll be persuaded its usefulness outweighs the small cost.

I'm also interested to see how easy it is to do post-mortem debugging w/o modifying fire, e.g. by try/excepting the call to Fire and using pdb in the except block there.

@yarikoptic
Copy link
Contributor

by try/excepting the call to Fire and using pdb in the except block there.

never heard about Fire before (used argparse and click extensively), was told about it, saw this issue which loved since nearly the first thing I always need to add such --pdb or --dbg flag. And indeed it is possible to make it all work "manually", and even without try/except, just by overloading sys.excepthook, e.g. how we did it here https://github.com/datalad/datalad/blob/7a5b58d74bccee55eeb2a04d6f1a86621e4b1e74/datalad/cli/utils.py#L11 .

But even though this easy, I think making it even easier to enable such "debug" option, would be quite cool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants