Strip the url before processing it #1564

Merged
merged 2 commits into from Jun 9, 2016

Conversation

Projects
None yet
3 participants
@Konubinix
Contributor

Konubinix commented Jun 9, 2016

This won't hurt and will help with some poorly formatted sites
including blank spaces around the url (e.g. the Previous link in a
dashboard make with CDash 2.0.2).


This change is Reviewable

Strip the url before processing it
This won't hurt and will help with some poorly formatted sites
including blank spaces around the url (e.g. the Previous link in a
dashboard make with CDash 2.0.2).
@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 9, 2016

Collaborator

Can you please add a test for this? You should be able to simply drop a file with a <!--target: ... --> comment to tests/end2end/data/hints/html and the tests will automatically pick it up. See e.g. simple.html for an example.

Collaborator

The-Compiler commented Jun 9, 2016

Can you please add a test for this? You should be able to simply drop a file with a <!--target: ... --> comment to tests/end2end/data/hints/html and the tests will automatically pick it up. See e.g. simple.html for an example.

@Konubinix

This comment has been minimized.

Show comment
Hide comment
@Konubinix

Konubinix Jun 9, 2016

Contributor

Like this ?

Contributor

Konubinix commented Jun 9, 2016

Like this ?

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 9, 2016

Collaborator

Looks good from the diff! Waiting for the tests to finish.

Don't worry if some unrelated tests on Travis/AppVeyor seem to fail, working on fixing them.

Collaborator

The-Compiler commented Jun 9, 2016

Looks good from the diff! Waiting for the tests to finish.

Don't worry if some unrelated tests on Travis/AppVeyor seem to fail, working on fixing them.

@The-Compiler The-Compiler merged commit a696100 into qutebrowser:master Jun 9, 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 9, 2016

Collaborator

So I noticed the test actually also passed without your fix, as it only tested clicking (i.e. f), but that didn't actually need the URL (and thus didn't call _resolve_url at all).

I fixed that by adding a BDD-style test to try with :hint links run: 288744c

Thanks!

Collaborator

The-Compiler commented Jun 9, 2016

So I noticed the test actually also passed without your fix, as it only tested clicking (i.e. f), but that didn't actually need the URL (and thus didn't call _resolve_url at all).

I fixed that by adding a BDD-style test to try with :hint links run: 288744c

Thanks!

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