Match partial results in completion (#1386) #1460

Merged
merged 2 commits into from Jun 6, 2016

Conversation

Projects
None yet
4 participants
@adamwethington7
Contributor

adamwethington7 commented Apr 27, 2016

Issue #1386

If there's only one result (I tried it with :bac for :back as Jumblemuddle mentioned), hitting enter will replace the command with the only suggested completion.


This change is Reviewable

@adamwethington7

This comment has been minimized.

Show comment
Hide comment
@adamwethington7

adamwethington7 Apr 27, 2016

Contributor

My bad. I'll get on the build failures.

Contributor

adamwethington7 commented Apr 27, 2016

My bad. I'll get on the build failures.

qutebrowser/commands/runners.py
@@ -172,6 +172,20 @@ def parse(self, text, *, aliases=True, fallback=False, keep=False):
return self.parse(new_cmd, aliases=False, fallback=fallback,
keep=keep)
try:
+

This comment has been minimized.

@The-Compiler

The-Compiler Apr 28, 2016

Collaborator

Is there a reason this is all inside the try:-block? I think it'd make more sense to only catch KeyError in the cmd = cmdutils.cmd_dict[cmdstr] line, but I might be missing something.

@The-Compiler

The-Compiler Apr 28, 2016

Collaborator

Is there a reason this is all inside the try:-block? I think it'd make more sense to only catch KeyError in the cmd = cmdutils.cmd_dict[cmdstr] line, but I might be missing something.

qutebrowser/commands/runners.py
+ the given command with the match.
+ Ex: If they type "bac" and the only completion is "back",
+ turn the command into "back".
+ """

This comment has been minimized.

@The-Compiler

The-Compiler Apr 28, 2016

Collaborator

Please use a normal comment - docstrings are intended as documentations at the start of a function/class

@The-Compiler

The-Compiler Apr 28, 2016

Collaborator

Please use a normal comment - docstrings are intended as documentations at the start of a function/class

qutebrowser/commands/runners.py
+
+ matches = []
+ for valid_command in cmdutils.cmd_dict.keys():
+ if valid_command.find(cmdstr) == 0:

This comment has been minimized.

@The-Compiler

The-Compiler Apr 28, 2016

Collaborator

This can be written as if cmdstr in valid_command instead

@The-Compiler

The-Compiler Apr 28, 2016

Collaborator

This can be written as if cmdstr in valid_command instead

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Apr 28, 2016

Collaborator

Thanks! I added a few line comments.

I'm also wondering if it would be better to add a keyword argument to activate that behaviour to make sure it's only active with the commandline - I think currently you'll get the partial matching even when e.g. spawning commands from a userscript, which I think is undesirable?

Collaborator

The-Compiler commented Apr 28, 2016

Thanks! I added a few line comments.

I'm also wondering if it would be better to add a keyword argument to activate that behaviour to make sure it's only active with the commandline - I think currently you'll get the partial matching even when e.g. spawning commands from a userscript, which I think is undesirable?

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Apr 30, 2016

Current coverage is 77.55%

Merging #1460 into master will increase coverage by +<.01%

@@             master      #1460   diff @@
==========================================
  Files           104        104          
  Lines         14803      14809     +6   
  Methods           0          0          
  Messages          0          0          
  Branches       2362       2365     +3   
==========================================
+ Hits          11478      11484     +6   
  Misses         2895       2895          
  Partials        430        430          

Powered by Codecov. Last updated by 0a76a75...fc0e986

Current coverage is 77.55%

Merging #1460 into master will increase coverage by +<.01%

@@             master      #1460   diff @@
==========================================
  Files           104        104          
  Lines         14803      14809     +6   
  Methods           0          0          
  Messages          0          0          
  Branches       2362       2365     +3   
==========================================
+ Hits          11478      11484     +6   
  Misses         2895       2895          
  Partials        430        430          

Powered by Codecov. Last updated by 0a76a75...fc0e986

@adamwethington7

This comment has been minimized.

Show comment
Hide comment
@adamwethington7

adamwethington7 Apr 30, 2016

Contributor

Hello,

I've fixed the comments and fixed some of the issues that pylint and flake8 pointed out with tox. I moved the code to a different method as pylint was advising that I had too many branches.

As far as the userscripts, I tried a couple of userscripts and had no problems with executing them.

With the if cmdstr in valid_command, I had taken the issue to mean that they wanted it to autocomplete whatever had been typed. If I used if cmdstr in valid_command, it selects string matches in strings that don't begin with what's been typed. If you want, I can change it to if cmdstr in valid_command anyway though, but that's why I had used String.find() instead of using in.

What can I do about the unexpected coverage changes that codecov reported? I'm fairly new to this.

Contributor

adamwethington7 commented Apr 30, 2016

Hello,

I've fixed the comments and fixed some of the issues that pylint and flake8 pointed out with tox. I moved the code to a different method as pylint was advising that I had too many branches.

As far as the userscripts, I tried a couple of userscripts and had no problems with executing them.

With the if cmdstr in valid_command, I had taken the issue to mean that they wanted it to autocomplete whatever had been typed. If I used if cmdstr in valid_command, it selects string matches in strings that don't begin with what's been typed. If you want, I can change it to if cmdstr in valid_command anyway though, but that's why I had used String.find() instead of using in.

What can I do about the unexpected coverage changes that codecov reported? I'm fairly new to this.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler May 1, 2016

Collaborator

As far as the userscripts, I tried a couple of userscripts and had no problems with executing them.

Yeah, my point is that an userscript now can also do e.g. echo ba > "$QUTE_FIFO" and that'll work. I'd prefer to have this behaviour for the interactive commandline only.

With the if cmdstr in valid_command, I had taken the issue to mean that they wanted it to autocomplete whatever had been typed. If I used if cmdstr in valid_command, it selects string matches in strings that don't begin with what's been typed. If you want, I can change it to if cmdstr in valid_command anyway though, but that's why I had used String.find() instead of using in.

Ah! I think this is equivalent to if valid_command.startswith(cmdstr): then? That'd be much clearer 😉

What can I do about the unexpected coverage changes that codecov reported? I'm fairly new to this.

You can ignore them. I originally turned off codecov reporting for pull requests as things aren't quite ready for that yet, but it seems like they changed their configuration recently.

Collaborator

The-Compiler commented May 1, 2016

As far as the userscripts, I tried a couple of userscripts and had no problems with executing them.

Yeah, my point is that an userscript now can also do e.g. echo ba > "$QUTE_FIFO" and that'll work. I'd prefer to have this behaviour for the interactive commandline only.

With the if cmdstr in valid_command, I had taken the issue to mean that they wanted it to autocomplete whatever had been typed. If I used if cmdstr in valid_command, it selects string matches in strings that don't begin with what's been typed. If you want, I can change it to if cmdstr in valid_command anyway though, but that's why I had used String.find() instead of using in.

Ah! I think this is equivalent to if valid_command.startswith(cmdstr): then? That'd be much clearer 😉

What can I do about the unexpected coverage changes that codecov reported? I'm fairly new to this.

You can ignore them. I originally turned off codecov reporting for pull requests as things aren't quite ready for that yet, but it seems like they changed their configuration recently.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler May 1, 2016

Collaborator

What also would be nice is if you could add a test for this. Something like this in tests/integration/features/misc.feature should work:

    Scenario: Partial commandline matching (#1386)
        When I run :message-i "Hello World"
        Then the message "Hello World" should be shown

A smaller/faster unittest should also be easy to add here, check the existing ones in tests/unit/commands/test_runners.py.

Collaborator

The-Compiler commented May 1, 2016

What also would be nice is if you could add a test for this. Something like this in tests/integration/features/misc.feature should work:

    Scenario: Partial commandline matching (#1386)
        When I run :message-i "Hello World"
        Then the message "Hello World" should be shown

A smaller/faster unittest should also be easy to add here, check the existing ones in tests/unit/commands/test_runners.py.

@The-Compiler The-Compiler changed the title from Proposed addition for issue #1386 to Match partial results in completion (#1386) May 4, 2016

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler May 15, 2016

Collaborator

Any update on this, @adamwethington7 😄

Collaborator

The-Compiler commented May 15, 2016

Any update on this, @adamwethington7 😄

@The-Compiler The-Compiler merged commit 5eea9d0 into qutebrowser:master Jun 6, 2016

4 of 5 checks passed

codecov/changes 6 files have unexpected coverage changes not found in diff.
Details
codecov/patch 100% of diff hit (target 77.54%)
Details
codecov/project 77.88% (+22.46%) compared to 0a76a75
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 6, 2016

Collaborator

I now merged this with some adjustments:

  • 4a7a2e6 Only do partial matching with main CommandParser
  • 2f60073 bdd: Allow to run invalid commands via quteproc
  • c9d85d3 bdd: Add tests for partial commandline matching
  • 5205723 Add unittests for partial command parsing

Thanks!

Collaborator

The-Compiler commented Jun 6, 2016

I now merged this with some adjustments:

  • 4a7a2e6 Only do partial matching with main CommandParser
  • 2f60073 bdd: Allow to run invalid commands via quteproc
  • c9d85d3 bdd: Add tests for partial commandline matching
  • 5205723 Add unittests for partial command parsing

Thanks!

@adamwethington7

This comment has been minimized.

Show comment
Hide comment
@adamwethington7

adamwethington7 Aug 22, 2016

Contributor

Hello,

I'm sorry for my absence. I had a pretty unexpected situation come up out
of nowhere that needed a lot of my attention. I didn't mean to completely
abandon it like that. Sorry for having to put that burden on you, it wasn't
intentional. Thanks for being kind.

Adam

On Mon, Jun 6, 2016 at 10:24 AM, Florian Bruhin notifications@github.com
wrote:

I now merged this with some adjustments:

Thanks!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1460 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AI6LWfXDuYUArgj0MacvqAQ5H3akVL-gks5qJC2ZgaJpZM4IRWYb
.

Contributor

adamwethington7 commented Aug 22, 2016

Hello,

I'm sorry for my absence. I had a pretty unexpected situation come up out
of nowhere that needed a lot of my attention. I didn't mean to completely
abandon it like that. Sorry for having to put that burden on you, it wasn't
intentional. Thanks for being kind.

Adam

On Mon, Jun 6, 2016 at 10:24 AM, Florian Bruhin notifications@github.com
wrote:

I now merged this with some adjustments:

Thanks!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1460 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AI6LWfXDuYUArgj0MacvqAQ5H3akVL-gks5qJC2ZgaJpZM4IRWYb
.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Aug 23, 2016

Collaborator

@adamwethington7 No worries! Thanks for the follow up, I appreciate it. Hope things are looking better (assuming it was the bad kind of unexpected) for you again. 😉

Collaborator

The-Compiler commented Aug 23, 2016

@adamwethington7 No worries! Thanks for the follow up, I appreciate it. Hope things are looking better (assuming it was the bad kind of unexpected) for you again. 😉

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