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

Alias multi commands #1577

Merged
merged 4 commits into from Jun 30, 2016

Conversation

Projects
None yet
4 participants
@mgoral
Contributor

mgoral commented Jun 12, 2016

This is a fix for #956.

With recent addition of history-clear command I feel that it would be extremely useful to have an alias for history-clear ;; quit. However above bug prevented that. I fixed that by simply moving a logic of checking if a given command is an alias from CommandRunner.parse() to CommandRunner.parse_all().

Please review the changes and tell how you feel about them. Thanks. :)


This change is Reviewable

Michał Góral added some commits Jun 12, 2016

Michał Góral
Moved searching for aliases to CommandRunner.parse_all()
This addresses issue with having alias for multiple commands splitted by ';;'.

See: qutebrowser/qutebrowser#956.
Michał Góral
Michał Góral
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 12, 2016

Current coverage is 81.47%

Merging #1577 into master will increase coverage by 0.04%

@@             master      #1577   diff @@
==========================================
  Files           106        106          
  Lines         15373      15370     -3   
  Methods           0          0          
  Messages          0          0          
  Branches       2466       2465     -1   
==========================================
+ Hits          12517      12522     +5   
+ Misses         2386       2378     -8   
  Partials        470        470          

Powered by Codecov. Last updated by cfe360b...4b883c0

codecov-io commented Jun 12, 2016

Current coverage is 81.47%

Merging #1577 into master will increase coverage by 0.04%

@@             master      #1577   diff @@
==========================================
  Files           106        106          
  Lines         15373      15370     -3   
  Methods           0          0          
  Messages          0          0          
  Branches       2466       2465     -1   
==========================================
+ Hits          12517      12522     +5   
+ Misses         2386       2378     -8   
  Partials        470        470          

Powered by Codecov. Last updated by cfe360b...4b883c0

Show outdated Hide outdated tests/unit/commands/test_runners.py
def mock_get(section, *args, **kwargs):
assert section == "aliases"
return alias_dict.get(*args, **kwargs)
monkeypatch.setattr("qutebrowser.config.config.get", mock_get)

This comment has been minimized.

@The-Compiler

The-Compiler Jun 13, 2016

Collaborator

I'm not sure if I ever used it for a list in the config (like the aliases section is), but I think you can simplify this by getting a config_stub fixture and simply doing config_stub.data = {'aliases': {'alias_name': cmdline_test.cmd}}.

@The-Compiler

The-Compiler Jun 13, 2016

Collaborator

I'm not sure if I ever used it for a list in the config (like the aliases section is), but I think you can simplify this by getting a config_stub fixture and simply doing config_stub.data = {'aliases': {'alias_name': cmdline_test.cmd}}.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 13, 2016

Collaborator

Thank you! I added a small comment, but this looks good otherwise I think.

Collaborator

The-Compiler commented Jun 13, 2016

Thank you! I added a small comment, but this looks good otherwise I think.

@mgoral

This comment has been minimized.

Show comment
Hide comment
@mgoral

mgoral Jun 13, 2016

Contributor

I haven't noticed that fixture before. It looks a lot cleaner so I'll give it a try later today. :)

Contributor

mgoral commented Jun 13, 2016

I haven't noticed that fixture before. It looks a lot cleaner so I'll give it a try later today. :)

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 13, 2016

Collaborator

Cool! Reminds me I really should write some more documentation on writing tests...

Collaborator

The-Compiler commented Jun 13, 2016

Cool! Reminds me I really should write some more documentation on writing tests...

@mgoral

This comment has been minimized.

Show comment
Hide comment
@mgoral

mgoral Jun 14, 2016

Contributor

Well, this one is actually finished, pylint was failing due to #1583 so if there are no more comments, it's ready for merge. :)

Contributor

mgoral commented Jun 14, 2016

Well, this one is actually finished, pylint was failing due to #1583 so if there are no more comments, it's ready for merge. :)

"""Get an alias from the config.
Args:
text: The text to parse.
default : Default value to return when alias was not found. By
default it is set to None.

This comment has been minimized.

@The-Compiler

The-Compiler Jun 30, 2016

Collaborator

nitpick: No point in stating this here as there's already default=None above 😉 I'll fix it up before merging unless other things come up while reviewing.

@The-Compiler

The-Compiler Jun 30, 2016

Collaborator

nitpick: No point in stating this here as there's already default=None above 😉 I'll fix it up before merging unless other things come up while reviewing.

@The-Compiler The-Compiler merged commit 9b394e4 into qutebrowser:master Jun 30, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
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 30, 2016

Collaborator

Sorry for the delay, and thanks! Merged

Collaborator

The-Compiler commented Jun 30, 2016

Sorry for the delay, and thanks! Merged

@mgoral mgoral deleted the mgoral:alias-multi-commands branch Jun 30, 2016

The-Compiler added a commit that referenced this pull request Jul 27, 2016

Handle empty command in CommandRunner.parse_all
Sicne we now call self._get_alias there, we also need to make sure it's
not an empty string before that.

Introduced in #1577. Fixes #1690.

The-Compiler added a commit that referenced this pull request Jul 27, 2016

Handle empty command in CommandRunner.parse_all
Sicne we now call self._get_alias there, we also need to make sure it's
not an empty string before that.

Introduced in #1577. Fixes #1690.

The-Compiler added a commit that referenced this pull request Aug 5, 2016

The-Compiler added a commit that referenced this pull request Aug 7, 2016

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