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

Management of KeyboardInterrupt in cmd.py #45635

Closed
stephbul mannequin opened this issue Oct 18, 2007 · 23 comments
Closed

Management of KeyboardInterrupt in cmd.py #45635

stephbul mannequin opened this issue Oct 18, 2007 · 23 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@stephbul
Copy link
Mannequin

stephbul mannequin commented Oct 18, 2007

BPO 1294
Nosy @gvanrossum, @avassalotti, @ngie-eign
Files
  • cmd.py
  • cmd.py.keyboardinterrupt.patch
  • cmd.diff
  • keyboardcmd.py
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2009-11-13.15:56:02.954>
    created_at = <Date 2007-10-18.12:51:56.450>
    labels = ['type-feature', 'library']
    title = 'Management of KeyboardInterrupt in cmd.py'
    updated_at = <Date 2012-10-27.20:15:32.395>
    user = 'https://bugs.python.org/stephbul'

    bugs.python.org fields:

    activity = <Date 2012-10-27.20:15:32.395>
    actor = 'Philip.Zerull'
    assignee = 'none'
    closed = True
    closed_date = <Date 2009-11-13.15:56:02.954>
    closer = 'gvanrossum'
    components = ['Library (Lib)']
    creation = <Date 2007-10-18.12:51:56.450>
    creator = 'stephbul'
    dependencies = []
    files = ['8562', '8566', '8568', '27752']
    hgrepos = []
    issue_num = 1294
    keywords = ['patch']
    message_count = 23.0
    messages = ['56524', '56529', '56551', '56557', '56561', '56606', '58168', '58173', '58174', '58175', '58498', '59904', '59906', '59970', '59976', '59980', '95051', '95146', '95174', '95177', '95179', '145854', '173968']
    nosy_count = 7.0
    nosy_names = ['gvanrossum', 'isandler', 'draghuram', 'alexandre.vassalotti', 'stephbul', 'ngie', 'Philip.Zerull']
    pr_nums = []
    priority = 'low'
    resolution = 'rejected'
    stage = 'test needed'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue1294'
    versions = ['Python 2.7', 'Python 3.2']

    @stephbul
    Copy link
    Mannequin Author

    stephbul mannequin commented Oct 18, 2007

    According to me, the Ctrl-C is not managed correctly in cmd.py. Ctrl-C
    generates a a KeyboardInterrupt exceptions, and only EOFError is
    managed. I propose to manage KeyboardInterrupt on line 130:
    print 'are you sure you want to exit? y/n'
    answer =''
    while (answer != 'y') & (answer != 'n'):
    answer = raw_input()
    if answer == 'y':
    exit(0)

    Here is attached my new cmd.py
    Hope ti will help.

    @stephbul stephbul mannequin added the type-bug An unexpected behavior, bug, or error label Oct 18, 2007
    @gvanrossum
    Copy link
    Member

    Would you mind submitting a patch instead of a whole new file?

    @stephbul
    Copy link
    Mannequin Author

    stephbul mannequin commented Oct 19, 2007

    Hello,
    Here is my patch for cmd.py
    Regards
    stephbul

    @gvanrossum
    Copy link
    Member

    Hmm... I don't think this is the right thing to do. The code is broken
    in several ways. I recommend you find someone to help you come up with
    a better patch in comp.lang.python.

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Oct 19, 2007

    I tested cmd.py on Linux and two things (including the one reported by
    OP) looked odd to me.

    1. CTRL-D produces a message "*** Unknown syntax: EOF".
    2. CTRL-C produces a KeyboardInterrupt exception and the session terminates.

    I am attaching a patch that changes the behaviour to a more typical one
    (at least, in IMHO). It terminates the session on CTRL-D and it just
    ignores CTRL-C. Both of these can be overridden, if required. If the
    patch is accepted, a doc change would be required in addition to the
    change in doc string. I tested the patch on Linux and Windows.

    @stephbul
    Copy link
    Mannequin Author

    stephbul mannequin commented Oct 20, 2007

    Well, I made it with a diff -ruN, it works fine on my ubuntu.
    It is only a ctrl-C management only, not a ctrl-D.
    What do you mean by broken?
    Regards.
    Stephbul

    2007/10/19, Guido van Rossum <report@bugs.python.org>:

    Guido van Rossum added the comment:

    Hmm... I don't think this is the right thing to do. The code is broken
    in several ways. I recommend you find someone to help you come up with
    a better patch in comp.lang.python.

    ----------
    keywords: +patch


    Tracker <report@bugs.python.org>
    <http://bugs.python.org/issue1294\>


    @avassalotti
    Copy link
    Member

    First, I would like to say thank you both for spending your time trying
    to do a contribution to Python.

    However, I believe the current behavior of cmd.py is correct. The module
    documentation states clearly that "End of file on input is processed as
    the command 'EOF'." For the KeyboardInterrupt issue, it thinks the
    exception should be handled by the application with a try-statement. For
    example,

       >>> import sys, cmd
       >>> c = cmd.Cmd()
       >>> try:
       ...     c.cmdloop()
       ... except KeyboardInterrupt:
       ...     print "\nGot keyboard interrupt. Exiting..."
       ...     sys.exit(0)
       ...
       (Cmd) ^C
       Got keyboard interrupt. Exiting...

    I am closing and marking this bug as rejected. If you feel this is
    inappropriate, please request with an appropriate explanation to reopen it.

    @avassalotti avassalotti added the stdlib Python modules in the Lib dir label Dec 4, 2007
    @avassalotti avassalotti added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Dec 4, 2007
    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Dec 4, 2007

    My patch adds very sensible default behaviour when Ctrl-C or Ctrl-D
    are entered. It follows the tradition of many unix programs such as
    bash and bc which exit on Ctrl-D and ignore Ctrl-c in one way or
    another. Most importantly, a user can always override the behaviour
    but I expect the added functionality would suffice in most if not all
    cases. Do you mind opening the issue in the hope that we can have more
    comments?

    @gvanrossum
    Copy link
    Member

    I will look into this for 2.6. No promises. Somebody ought to check
    whether this does not cause problems for pdb.

    @gvanrossum gvanrossum reopened this Dec 4, 2007
    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Dec 4, 2007

    I will look into this for 2.6. No promises. Somebody ought to check
    whether this does not cause problems for pdb.

    Thanks. I will check about pdb tomorrow.

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Dec 12, 2007

    "Tomorrow" took a while to come by :-)

    With out the patch, pdb prints this on Ctrl-C:

    "KeyboardInterrupt
    Uncaught exception. Entering post mortem debugging
    Running 'cont' or 'step' will restart the program"

    With the patch, pdb prompt appears again. Ctrl-D change doesn't effect
    pdb because do_EOF() is already implemented. My test was on SuSE 10 Linux.

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Jan 14, 2008

    bugday task?

    @gvanrossum
    Copy link
    Member

    To mark things for the bugday, set the 'easy' keyword.

    However, this particular one is IMO too subtle for a bugday, witness the
    discussion here. Perhaps a bugday could come up with an ultimate patch,
    but I'd be hesitant to submit it without having reviewed it personally.

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Jan 15, 2008

    Ok. BTW, can I get tracker permissions? I will try to check old bugs to
    update their information and if required, close them.

    @gvanrossum
    Copy link
    Member

    I've added developer status to your username. Let me know if it doesn't
    work.

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Jan 15, 2008

    I've added developer status to your username. Let me know if it doesn't
    work.

    It does. Thanks.

    @isandler
    Copy link
    Mannequin

    isandler mannequin commented Nov 9, 2009

    Is not this patch backward incompatible?

    E.g any cmd-based application which expects Ctrl-C to propagate to the
    top level will be broken by this patch.

    As for pdb, I don't think pdb will benefit from this patch: as I believe
    that pdb needs something along the lines of patch bpo-7245 for Ctrl-C
    (temporary interrupt of execution with ability to resume similar to what
    gdb does)

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Nov 11, 2009

    On Sun, Nov 8, 2009 at 8:22 PM, Ilya Sandler <report@bugs.python.org> wrote:

    Is not this patch backward incompatible?

    E.g any cmd-based application which expects Ctrl-C to propagate to the
    top level will be broken by this patch.

    But currently, CTRL-C terminates the session instead of propagating
    upstream. The only way it would do so is if do_KeyboardInterrupt() is
    implemented in which case, the behaviour would remain same even with
    the patch.

    @isandler
    Copy link
    Mannequin

    isandler mannequin commented Nov 13, 2009

    But currently, CTRL-C terminates the session instead of propagating
    upstream

    I am not sure I understand: currently Ctrl-C generates a
    KeyboardInterrupt, which can be caught by the application which can
    then decide how to proceed (in particular it can start another command
    loop or exit with a meaningful message or anything else).

    This patch would suppress KeyboardInterrupt and thus interfere with such
    applications. Or am I missing something?

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Nov 13, 2009

    I am not sure I understand: currently Ctrl-C generates a
    KeyboardInterrupt, which can be caught by the application which can
    then decide how to proceed (in particular it can start another command
    loop or exit with a meaningful message or anything else).

    This patch would suppress KeyboardInterrupt and thus interfere with such
    applications. Or am I missing something?

    I checked the patch and tested with python from trunk. You are right
    that the patch catches KeyboardInterrupt thus interfering with any
    applications that expect it to be propagated upstream. Perhaps, this
    can be made conditional so that we can keep both behaviors.

    But CTRL-D processing doesn't suffer from any backwards compatible
    issues and that part of the patch should be able to be applied safely.

    @gvanrossum
    Copy link
    Member

    I don't think this is a good idea.

    @ngie-eign
    Copy link
    Mannequin

    ngie-eign mannequin commented Oct 18, 2011

    I realize that this bug is closed, but I just had a comment to make.

    Handling EOF is simple:

    def do_EOF(self, arg):
        pass

    For my purposes I want to raise an EOFError so I can trickle up the chain to the appropriate caller because I'm coding a CLI where I have a nested set of commands, e.g.

    command subcommand_0
    command subcommand_1

    I'd like the behavior to match what's done in Cisco IOS or IronPort's CLI (to some degree).

    The only part that's annoying is that I have to hide do_EOF in the help and completion output, otherwise the user will see the handler when completing or running help, but I'll bring that up in another issue.

    @PhilipZerull
    Copy link
    Mannequin

    PhilipZerull mannequin commented Oct 27, 2012

    Hello,

    Being of a similar mindset to draghuram on the do_KeyboardInterrupt idea and thought I'd implement it as a subclass. While this probably wasn't fully implemented correctly, I think it provides an interesting solution to stephbul's frustrations and won't break anything.

    I'm not suggesting that this be included in the standard library but just thought you all might find it interesting.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants