click-contrib / click-repl

Subcommand REPL for click apps
MIT License
215 stars 41 forks source link

Miscellaneous fixes and improvements #22

Closed bobwhitelock closed 7 years ago

bobwhitelock commented 7 years ago

This PR contains a couple of small tweaks to click-repl to better support my use case (while preserving the current default behaviour) and to fix what doesn't seem useful behaviour to me, allowing the creation of nested REPLs. See the individual commits for full details.

I'm happy to make further changes to this if you think any are needed before merging.

Thanks!

untitaker commented 7 years ago

LGTM please add a testcase though (just testing the completion in the way already done)

bobwhitelock commented 7 years ago

Hi - not sure what you mean about testing the completion as is already done. If I change tests/test_basic.py to something like this (assuming this is what you mean):

import click
from click_repl import register_repl, ClickCompleter
from prompt_toolkit.document import Document

def test_completion():
    @click.group()
    def root_command():
        pass

    @root_command.group()
    def first_level_command():
        pass

    @first_level_command.command()
    def second_level_command_one():
        pass

    @first_level_command.command()
    def second_level_command_two():
        pass

    register_repl(root_command, name='myrepl')

    c = ClickCompleter(root_command)

    root_completions = list(c.get_completions(Document(u'')))
    assert set(x.text for x in root_completions) == set([
        u'first_level_command',
    ])

    first_level_completions = list(
        c.get_completions(Document(u'first_level_command ')))

    assert set(x.text for x in first_level_completions) == set([
        u'second_level_command_one',
        u'second_level_command_two'
    ])

This fails due to:

E       assert set(['first_l...d', 'myrepl']) == set(['first_level_command'])
E         Extra items in the left set:
E         'myrepl'

This doesn't work as the repl command is already removed from the context by the time we pass the group to the ClickCompleter, and the completer does nothing special to filter out the repl command, so this isn't testing the new functionality.

So presumably we need to test this on a different level if we're going to. I'm not sure of the best way to do this right now; I may look again tomorrow. Let me know if you have a suggestion, or if I've misinterpreted what you meant about how to test this.

untitaker commented 7 years ago

Thanks. I am not sure how to add tests either, but I guess they're not urgently needed.

bobwhitelock commented 7 years ago

Thanks for merging :). Yeah, it would be nice to have test coverage for this but as there's not much test coverage generally for this library it sounds like it may be quite involved to add this.