Add more variables (cont.) #4652

jgkamat · 2019-03-17T05:31:30Z

This is a continuation of #1937

I'm going to ignore the extremely inefficient and unnecessary dict creation/loop in the interest of making the diff as small as possible.

Together with #4651, will close #4647.

This change is

This adds more variables, in particular: {url:domain} {url:auth} {url:scheme} {url:user} {url:password} {url:host} {url:port} {url:path} {url:query}

Variables such as {url} and {selection} can now be explicitly escaped by prefixing them with a backslash: \{url} Only valid variables that would've been replaced are escaped, so '\{notavariable}' stays '\{notavariable}' See qutebrowser#1861

This reverts commit c6f51c4.

The-Compiler · 2019-03-19T08:49:31Z

tests/end2end/features/misc.feature

@@ -441,6 +441,49 @@ Feature: Various utility commands.
        And I run :message-info {clipboard}bar{url}
        Then the message "{url}barhttp://localhost:*/hello.txt" should be shown

+    Scenario: Variable {url:pretty}


Any chance you could take a quick look into what it'd take to make those unit rather than end2end tests? If it ends up not being possible easily then those are fine, but if it's possible, I'd strongly prefer that.

I don't think I can make it unit tests (because most variable expansion requires a tabbedbrowser, which I'm not entirey sure I can properly mock), but I did get it to a python e2e test, does that sound ok?

The-Compiler · 2019-03-19T08:49:55Z

tests/end2end/features/misc.feature

+        Then the message "foo" should be shown
+
+# Tests for HTTP basic auth variables are missing here, pytest-bdd bug
+# https://github.com/The-Compiler/qutebrowser/pull/1921#issuecomment-244695985


Fixed in September 2016, so it should be possible to add those.

The-Compiler · 2019-03-19T08:54:30Z

I'm going to ignore the extremely inefficient and unnecessary dict creation/loop

Can you elaborate? I'm getting a bit worried we're starting to micro-optimize things which don't actually make a difference in practice (a few microseconds or whatever when executing a command really doesn't make a difference). How is it "extremely" inefficient? Why is it unnecessary? I'm really grateful for your performance work in the areas where it's really needed (and there are some of those, granted), but when deciding between readability/simplicity vs. performance, I'm still going to default to the former, especially when there's no evidence that it makes a difference in practice.

jgkamat · 2019-03-19T17:08:02Z

Florian Bruhin writes:

Can you elaborate? I'm getting a bit worried we're starting to micro-optimize things which don't actually make a difference in practice (a few microseconds or whatever when executing a command really doesn't make a difference). How is it "extremely" inefficient? Why is it unnecessary? I'm really grateful for your performance work in the areas where it's really needed (and there are some of those, granted), but when deciding between readability/simplicity vs. performance, I'm still going to default to the former, especially when there's no evidence that it makes a difference in practice.

Firstly, for me, the inefficient code (just building and iterating over the mapping, the things that could be eliminated by declaring a global and passing the tabbedbrowser in) had an overhead of ~0.01 ms on my machine. The new code has an overhead of ~0.04 ms. I really don't like introducing performance regressions, and a 0.03ms regression per command is pretty large. I run a lot of commands per page, which means I will spend a comparable amount of time in this code as in my python adblocker on lightweight pages (which is currently about 0.2 ms per request overhead. And of course, this is all on my ~months old, 1k$ laptop. If this work was justified, I wouldn't mind too much, but to me, this is just pure wasted cycles. This code sets a bad example of using a mapping as a list. Maybe it's just me, but sending code off to (I'm guessing) ~1000s of machines to run pointless loops over hashmaps dosen't feel right - it's easily preventable and wastes the user's resources. Even worse, a new programmer might come across this code someday, and think that looping over mappings is the proper way to use them. When a new contributor adds a new expansion, they will be increasing the regression even further (at no fault of their own). I don't really see how this particular structure saves anyone any time, whareas I have wasted a LOT of my time profiling/scouring the codebase only to find many instances of this issue (rather than more interesting discoveries), and I don't want to do that forever. Fixing only the hottest instances would take much, much more time than just doing it properly everywhere, and result in a slower final product. Does it make a difference in this case? Probably not, but that's only because there's a much larger version of this same problem (the config system iterating over dicts and keybindings iterating over the binding dict) overshadowing everything else. Finally, in this particular case, I would say this version is more arcane. Having the dictionary inline dilutes the purpose of this function and makes it harder to understand. Declaring it as a constant in the module with a real name and docstring (not 'variables') would be much more readable and maintainable, imho.

user202729 · 2019-03-25T14:31:26Z

It's not exactly "extremely inefficient", but I can see no benefit in doing it - it doesn't make the code simpler, or easier to read, or something like that.

The-Compiler · 2019-04-17T15:13:03Z

Does it make a difference in this case? Probably not, but that's only because
there's a much larger version of this same problem (the config system
iterating over dicts and keybindings iterating over the binding dict)
overshadowing everything else.

Thanks for the explanation. Before this, I never realized that you regard this specific thing as a (recurring) problem - as far as I remember, you never told me 😉

I agree it's something which should be done rather sparingly, but in some cases there isn't really a better solution - like for matching keybindings IIRC, where the matching algorithm isn't just a fixed lookup.

Those things could be a list of tuples instead (because they aren't really used as a dict), but that wouldn't really change anything either, and just make the syntax more awkward, so I don't really see the gain. They probably should be more fitting data structures (perhaps a trie for keybindings?) though.

Either way, I'll try to keep an eye on it in the future.

As for this specific case:

Maybe it's just me, but sending code off to (I'm guessing) ~1000s of machines to run pointless loops over hashmaps dosen't feel right - it's easily preventable and wastes the user's resources.

It's not exactly "extremely inefficient", but I can see no benefit in doing it - it doesn't make the code simpler, or easier to read, or something like that.

So what would you (both) suggest instead? Note that:

If no replacement is used, commands should be able to execute without a tabbed_browser/URL being available.
Using {{url}} should escape the {url} variable.

Here's what I can think of:

Moving the variables dict and replace_variables into CommandRunner, and filling it up with the escaped replacements in __init__
For every replacement like url, add {url}: lambda: {url} hardcoded into the dict (with the cost of making it double as verbose)
Have a real commandline parser (the goal of Less special cases for commandline parsing #2017)

I'm open for suggestions - I'd probably go for 1. in the short term and 3. in the long term. 2. would avoid the loop, but it seems to be like that would indeed make the code harder to read (or at least more verbose).

The-Compiler

Some small issues with the tests, but I'll fix that up while merging.

The-Compiler · 2019-04-17T15:18:47Z

tests/end2end/misc/test_runners_e2e.py

+
+
+def test_command_expansion_clipboard(quteproc):
+    quteproc.send_cmd(':debug-set-fake-clipboard "{}"'.format('foo'))


Formatting with a constant string makes little sense...

The-Compiler · 2019-04-17T15:18:52Z

tests/end2end/misc/test_runners_e2e.py

+    command_expansion_base(
+        quteproc, '{clipboard}bar{url}',
+        "foobarhttp://localhost:*/hello.txt")
+    quteproc.send_cmd(':debug-set-fake-clipboard "{}"'.format('{{url}}'))


The-Compiler · 2019-04-17T15:19:15Z

tests/end2end/misc/test_runners_e2e.py

+
+
+def test_command_expansion_basic_auth(quteproc, server):
+    url = 'http://user1:password1@localhost:{port}/basic-auth/user1/password1' \


I prefer adding parens to backslash continuation

user202729 · 2019-04-17T15:20:36Z

@The-Compiler Regarding the "related" problem you mentioned: I implemented trie data structure for keybindings (https://paste.the-compiler.org/view/c15f385c (by the way the captcha says "type in the letters" while reCaptcha isn't that)), however the implementation is incomplete:

Partial key sequence suggestion or config bind/unbind (on_config_changed) still traverse the whole dict.
update method is not implemented (although this one is easy to implement, just loop over all items)

I may open a PR later, when it works properly and I can check that it does improve performance.

The-Compiler · 2019-04-17T15:29:37Z

@user202729 Nice! I opened a separate issue for that: #4721

@jgkamat and @kobezda: Thanks for the contribution!

jgkamat · 2019-04-18T04:31:02Z

Florian Bruhin writes:

- If no replacement is used, commands should be able to execute without a tabbed_browser/URL being available. - Using `{{url}}` should escape the `{url}` variable. Here's what I can think of: - Moving the `variables` dict and `replace_variables` into `CommandRunner`, and filling it up with the escaped replacements in `__init__` - For every replacement like `url`, add `{url}: lambda: {url}` hardcoded into the dict (with the cost of making it double as verbose) - Have a real commandline parser (the goal of #2017) I'm open for suggestions - I'd probably go for 1. in the short term and 3. in the long term. 2. would avoid the loop, but it seems to be like that would indeed make the code harder to read (or at least more verbose).

I'm not sure exactly what you meant by "should be able to execute without a tabbed_browser/URL being available". In this function, I think we're getting the tabbedbrowser/url every time (is there a reason to avoid that). I think we can get something much better by just lifting the logic that dosen't change into the root level (or CommandRunner, but that would be more invasive/confusing imo). Does http://paste.debian.net/plain/1078135 seem like it has any big issues? Naively timing it seems to make 'replace_variables' a little under an order of magnitude faster for me.

kobezda and others added 6 commits September 9, 2016 13:43

Add more variables

7c5a92c

This adds more variables, in particular: {url:domain} {url:auth} {url:scheme} {url:user} {url:password} {url:host} {url:port} {url:path} {url:query}

Add a way to escape variables

c6f51c4

Variables such as {url} and {selection} can now be explicitly escaped by prefixing them with a backslash: \{url} Only valid variables that would've been replaced are escaped, so '\{notavariable}' stays '\{notavariable}' See qutebrowser#1861

Add tests for new variables

abaeb21

Revert "Add a way to escape variables"

63244c7

This reverts commit c6f51c4.

Fix having mandatory URL for replacements

1ace678

Merge branch 'master' into HEAD

d5c4287

qutebrowser-bot added the status: needs review label Mar 17, 2019

jgkamat self-assigned this Mar 17, 2019

Fix pylint warning

671891c

The-Compiler requested changes Mar 19, 2019

View reviewed changes

The-Compiler removed the status: needs review label Mar 19, 2019

qutebrowser-bot added the status: needs review label Mar 20, 2019

jgkamat force-pushed the more-variables branch from 3597171 to 3aa5d64 Compare March 20, 2019 04:24

Switch to python e2e tests for variable expansion

700493a

jgkamat force-pushed the more-variables branch from 3aa5d64 to 700493a Compare March 21, 2019 00:58

The-Compiler mentioned this pull request Mar 25, 2019

Add a way to escape replacements in the commandline #1861

Closed

The-Compiler added the jay: silver label Mar 27, 2019

The-Compiler reviewed Apr 17, 2019

View reviewed changes

The-Compiler mentioned this pull request Apr 17, 2019

Consider better data structures for bindings #4721

Closed

The-Compiler merged commit 700493a into qutebrowser:master Apr 17, 2019

jgkamat deleted the more-variables branch April 18, 2019 03:26

This was referenced Apr 22, 2019

Build variable replacement dict on init #4731

Merged

Rename {title} to {current_title} #4733

Merged

arza-zara mentioned this pull request Oct 5, 2019

Add an option to yank the HTTP basic access authentification credentials with the URL #1848

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more variables (cont.) #4652

Add more variables (cont.) #4652

jgkamat commented Mar 17, 2019 •

edited by The-Compiler

The-Compiler Mar 19, 2019

jgkamat Mar 20, 2019

The-Compiler Mar 19, 2019

The-Compiler commented Mar 19, 2019

jgkamat commented Mar 19, 2019 via email

user202729 commented Mar 25, 2019 •

edited

The-Compiler commented Apr 17, 2019

The-Compiler left a comment

The-Compiler Apr 17, 2019

The-Compiler Apr 17, 2019

The-Compiler Apr 17, 2019

user202729 commented Apr 17, 2019 •

edited

The-Compiler commented Apr 17, 2019

jgkamat commented Apr 18, 2019 via email



		def test_command_expansion_clipboard(quteproc):
		quteproc.send_cmd(':debug-set-fake-clipboard "{}"'.format('foo'))



		def test_command_expansion_basic_auth(quteproc, server):
		url = 'http://user1:password1@localhost:{port}/basic-auth/user1/password1' \

Add more variables (cont.) #4652

Add more variables (cont.) #4652

Conversation

jgkamat commented Mar 17, 2019 • edited by The-Compiler

The-Compiler Mar 19, 2019

Choose a reason for hiding this comment

jgkamat Mar 20, 2019

Choose a reason for hiding this comment

The-Compiler Mar 19, 2019

Choose a reason for hiding this comment

The-Compiler commented Mar 19, 2019

jgkamat commented Mar 19, 2019 via email

user202729 commented Mar 25, 2019 • edited

The-Compiler commented Apr 17, 2019

The-Compiler left a comment

Choose a reason for hiding this comment

The-Compiler Apr 17, 2019

Choose a reason for hiding this comment

The-Compiler Apr 17, 2019

Choose a reason for hiding this comment

The-Compiler Apr 17, 2019

Choose a reason for hiding this comment

user202729 commented Apr 17, 2019 • edited

The-Compiler commented Apr 17, 2019

jgkamat commented Apr 18, 2019 via email

jgkamat commented Mar 17, 2019 •

edited by The-Compiler

user202729 commented Mar 25, 2019 •

edited

user202729 commented Apr 17, 2019 •

edited