wsdjeg / vim-fetch

Make Vim handle line and column numbers in file names with a minimum of fuss
http://www.vim.org/scripts/script.php?script_id=5089
MIT License
312 stars 17 forks source link

Add support for `file::test_method_name` #3

Closed blueyed closed 9 years ago

blueyed commented 9 years ago

py.test displays patterns like this. The test_method_name is used as a pattern for "/".

Do you think this makes sense, and is the implementation OK?

kopischke commented 9 years ago

Thanks a lot for the PR. Yes, I think it makes sense to integrate this, but no, I’m not overly fond of the hack that turns a:pos into a search instruction – I’d rather see that case covered by returning the match position found from parse()(EDIT: no can do, that is called before the new buffer is loaded. Will have to see about that).

I also can’t help thinking the search should be for something more specific than just the method name, which can, after all, turn up in comments, string literals and other unexpected places, but as I’m not familiar with py.test (Google was my friend, but the site is a bit much to absorb in a hurry), I’m wondering for how many languages a specific pattern would be needed, or if a tag search might be better suited…

kopischke commented 9 years ago

@blueyed so I have a tentative implementation that allows returning [Funcref, [arguments]] instead of numerical positions from parse(), which will then be call()ed by fetch#view#setpos(). Basically, your PR logic, but without the tight coupling to searching and the “magical” position String value (I’ve just pushed the blueyed-support-method-pattern branch, so you can have a look). What I need to complete this is your input on the file type question above; currently, the function supposed to return the final position is a no-op.

blueyed commented 9 years ago

Thanks for looking into this and improving upon my hack! :)

I also can’t help thinking the search should be for something more specific than just the method name

Agreed.

As for py.test it would have to match def\s+$name\s\( (a Python function), but I would not restrict it too much. Maybe including word boundaries is a good trade-off (\<$name\>), although that might still show up elsewhere. A tag search might be useful, with falling back to a normal search, if no tags are found.

kopischke commented 9 years ago

Ah well, if py.test is Python only, a simple (well, accounting for the inherent madness of Vim’s regex syntax) search pattern should do. If I am interpreting the language spec correctly, something along the lines of:

\m^\s*def\s\+\%(\\\n\s*\)*\zs$name\ze(

should do the trick. A tag search should not be needed as the format means “this function definition in this file”, or am I missing something?

blueyed commented 9 years ago

Yes, except that there could be optional whitespace before the opening parenthesis at the end.

kopischke commented 9 years ago

Oh yes, I forgot: Python, where whitespace is significant – except when it’s not ;). Thanks, I think I can rustle something up with this.

kopischke commented 9 years ago

@blueyed I’ve re-pushed the branch with a pattern jump implementation for py.test specs (Python files only). Could you give it a spin and report if it works and matches your expectations, please?

kopischke commented 9 years ago

@blueyed did you have a chance to check out the branch? I’ll push a new vim-stay release soon, and I’d like to have your input on if this should be included.

blueyed commented 9 years ago

I've just tried it.

% vi path/to/file.py::test_foo_bar

resulted in this error:

Error detected while processing function fetch#edit..fetch#setpos:
line    1:
E725: Calling dict function without Dictionary: 42

See https://github.com/kopischke/vim-fetch/pull/5 for a possible fix.

blueyed commented 9 years ago

With this fix it works good.

But I would not limit it to this specific use case only (python filetype, def pattern). But then again a less specific spec could be added later anyway.

kopischke commented 9 years ago

Ugh, I forgot about the self Dict argument in such call()s – good, er, call (no pun intended). I will have to look for the most elegant solution, as I’m a bit loath to subvert the parse() return semantics.

As to the specificity: the mechanism itself would be open to other spec formats, as you rightly remarked; it’s just this “pytest” spec that is limited to Python method definitions.

kopischke commented 9 years ago

I tweaked the Funcref handling logic a bit to handle anonymous functions, adapting your #6. Would you give the re-pushed branch another try?

blueyed commented 9 years ago

It works!

kopischke commented 9 years ago

@blueyed great to hear! I’ll merge it into the next release, then.

kopischke commented 9 years ago

@blueyed I have integrated the pytest method spec into the upcoming release; however, its implementation is a bit different from the branch you tested, as I refactored vim-fetch’s whole position setting logic. It would be great if you could test the 2.0.0 branch before I release it.

Note that due to the extensive refactoring, this is not directly based on your PR anymore, but I will credit you in the release notes as a matter of course. I hope that is OK with you – if not, please do say!

blueyed commented 9 years ago

@kopischke :+1: I've just tested the 2.0.0 branch, and it still works.

kopischke commented 9 years ago

Cool – thanks for the feedback!