laktak / extrakto

extrakto for tmux - quickly select, copy/insert/complete text without a mouse
MIT License
878 stars 45 forks source link

Add tests for urls and paths #5

Closed ivanalejandro0 closed 7 years ago

ivanalejandro0 commented 7 years ago

Hi there, I noticed a bug on the pattern matching, not supporting UPPER CASE urls. Also, I found out that there was no 'sftp' prefix on the regular expression for that matter.

I wondered which other cases I may be missing and I think that it would be a good idea to have tests to see what works and what doesn't. After starting with tests I realized that I had to make several changes to the code for them to work and be easily understandable.

I hope you don't mind the many changes I made to your code, if so, please let me know and I can fix whatever you think is best.

This doesn't require any extra dependency, you can run them as (for example) python -m tests/test_get_urls.

Here is an example output of what this PR adds:

➜ python -m tests/test_get_urls -v
test_match_HTTP (__main__.TestGetURLs) ... ok
test_match_ftp (__main__.TestGetURLs) ... ok
test_match_git (__main__.TestGetURLs) ... ok
test_match_home_path (__main__.TestGetURLs) ... ok
test_match_http (__main__.TestGetURLs) ... ok
test_match_https (__main__.TestGetURLs) ... ok
test_match_sftp (__main__.TestGetURLs) ... ok

----------------------------------------------------------------------
Ran 7 tests in 0.001s

OK

➜ python -m tests/test_get_paths -v
test_match_full_path (__main__.TestGetPaths) ... ok
test_match_tilde_path (__main__.TestGetPaths) ... ok

----------------------------------------------------------------------
Ran 2 tests in 0.001s

OK

If you want, you can use a test runner to simplify the task of running all the tests, I recommend you pytest, this is an output using it:

➜ pytest -v
============================= test session starts =============================
[...trimmed output...]

tests/test_get_paths.py::TestGetPaths::test_match_full_path PASSED
tests/test_get_paths.py::TestGetPaths::test_match_tilde_path PASSED
tests/test_get_urls.py::TestGetURLs::test_match_HTTP PASSED
tests/test_get_urls.py::TestGetURLs::test_match_ftp PASSED
tests/test_get_urls.py::TestGetURLs::test_match_git PASSED
tests/test_get_urls.py::TestGetURLs::test_match_home_path PASSED
tests/test_get_urls.py::TestGetURLs::test_match_http PASSED
tests/test_get_urls.py::TestGetURLs::test_match_https PASSED
tests/test_get_urls.py::TestGetURLs::test_match_sftp PASSED

========================== 9 passed in 0.02 seconds ===========================

Changes list:

maximbaz commented 7 years ago

Just FYI, since #2 the code in master doesn't use these regexes, and does not suffer from at least some of the issues that you are fixing here. Not saying that your PR is entirely useless, just wanted you be aware.

image

ivanalejandro0 commented 7 years ago

Hey @maximbaz thanks for the heads up!. I was checking the python code instead of the actual tmux/fzf behavior for those problems so I didn't realized.

They are matching because the plugin is extracting words (see extrakto.tmux), not urls.

See this example:

➜ echo "lowercase sftp://site.com and UPPERCASE HTTP://GOOGLE.COM" | python extrakto.py -u
ftp://site.com

# this is what tmux/fzf uses right now
➜ echo "lowercase sftp://site.com and UPPERCASE HTTP://GOOGLE.COM" | python extrakto.py -w
lowercase
sftp://site.com
UPPERCASE
HTTP://GOOGLE.COM

So, even though users (most likely) won't experience the problem I mentioned, these are valid url regexp fixes.

Cheers!

laktak commented 7 years ago

Thanks, that's more than I would have done ;)

@maximbaz I think I should add options to make this configurable.

maximbaz commented 7 years ago

Up to you of course, but honestly I haven't seen a single case yet when I would be disappointed by the results discovered with the current "words" approach. I'm still keeping an eye on it 😉

laktak commented 7 years ago

That's true but extrakto.py can be used without tmux as well to process text files were -p and -u are quite useful!

maximbaz commented 7 years ago

That is a good point that I've missed 👍