Add support for punctuation at the beginning

[aaronjensen]

I just added a few more tests and this doesn't actually work... in my$prize, $prize is still matched :/ I'd probably need to look at the first character to see if it is punctuation and if so omit the \b from the regex

[aaronjensen]

ok, give that a shot. it will probably be very slightly slower, but it's probably a fine tradeoff given the increased accuracy and support for things like $vars

[dirkwallenstein]

Thanks. This reveals at least two bugs.

(see vim doc complete-functions) The completion function is called in two different ways:

• First the function is called to find the start of the text to be completed.
• Later the function is called to actually find the matches.

Findstart and completion are indeed inconsistent here. But I would actually suggest to always anchor the findstart at word boundaries and make that official.

The reason is that definition and usage of an item don't always use the same prefix. For example shell variables or python decorators. So you would not find the definitions.

But more importantly, there is an issue for combining completion functions. They all have to return the same starting column in the first call because that is what will be replaced by Vim after filling the menu with the second call. So if localcomplete finds punctuations at the start it limits what other completion functions it can be combined with and therefore counters the main goal to some extent.

I think prefixes apply to specialized completion functions only. Completion functions can recognize the context in the first call, and provide appropriate matches in the second.

The escaping fixes a bug. If you do that for all compilations that could be a nice commit. Also, if you would like to anchor the findstart at word boundaries, please go ahead. Please add some descriptions in the commits if you want to do that. Also, it would be nice to document both with tests and thereby prevent regressions.

[aaronjensen]

But more importantly, there is an issue for combining completion functions. They all have to return the same starting column in the first call because that is what will be replaced by Vim after filling the menu with the second call. So if localcomplete finds punctuations at the start it limits what other completion functions it can be combined with and therefore counters the main goal to some extent.

This actually makes it consistent w/ the behavior of ctrl-p/n, which my understanding is what it is designed to mimic. To see this, set iskeyword+=$, disable acp, and try to autocomplete $something w/ ctrl--p.

For languages such as python or shell, one would simply not include that punctuation in your iskeyword. If you do not, the regex effectively acts as word boundary. This also fixes - prefixed things when iskeyword contains -.

In my opinion, localcomplete#AdditionalKeywordChars should default to &iskeyword and the - should be sorted to the end automatically along w/ this patch to make it more consistent w/ ctrl-p.

The escaping fixes a bug. If you do that for all compilations that could be a nice commit. Also, if you would like to anchor the findstart at word boundaries, please go ahead. Please add some descriptions in the commits if you want to do that. Also, it would be nice to document both with tests and thereby prevent regressions.

I can make them all behave consistently and improve the commit messages, but first I'm wondering what your thoughts are on the above. As for tests, I tried to add tests for the things I changed, are those tests insufficient? Is there a particular case you'd like to see?

[dirkwallenstein]

This actually makes it consistent w/ the behavior of ctrl-p/n, which my understanding is what it is designed to mimic. To see this, set iskeyword+=$, disable acp, and try to autocomplete $something w/ ctrl--p.

Hm, you are right.

Also, I remember that there is always only one completion picked for findstart. So, you would always pick the one that acquires context across calls (eg rope, which does not respect iskeyword AFAICT).

You don't need to sort when you escape, I believe.

[aaronjensen]

You don't need to sort when you escape, I believe.

I'm not escaping the punctuation, though it looks like I could escape punctuation as well, so that'd work. I'll fix it up to do that and fix up the other match types.

[dirkwallenstein]

Why are you using the non-capturing parentheses? Makes the pattern just more complicated.

[aaronjensen]

Why are you using the non-capturing parentheses? Makes the pattern just more complicated.

I didn't dig into it, but it didn't work when they captured. It makes it visually more complicated but it probably makes it (negligibly) faster.

[dirkwallenstein]

I don't think you need it at all. The doc says the assertion matches at the start of a string.

[aaronjensen]

From: http://docs.python.org/2/library/re.html#re.findall

re.findall(pattern, string, flags=0) Return all non-overlapping matches of pattern in string, as a list of strings. The string is scanned left-to-right, and matches are returned in the order found. If one or more groups are present in the pattern, return a list of groups; this will be a list of tuples if the pattern has more than one group. Empty matches are included in the result unless they touch the beginning of another match.

It changes format of the return which broke the consumer.

[aaronjensen]

please take a look @dirkwallenstein, I rebased and fixed up the other bugs I think.

[dirkwallenstein]

Create a commit for each logical change

You are doing much too much in one commit. How do people see what effort you did put into this when you are cramming all into one commit. But seriously, that is hard to review.

Commits I can spot while skimming. (Order and partition is not mandatory. See what makes sense when doing this)

• escape when creating patterns (fixes one bug; always fix one bug with one commit if possible)
• extract function (awesome, but make it one commit so that it is possible to see what changes if anything)
• Add support for punctuation prefixes
• the change in the return value. Explain that in a commit, not in a github comment.

This is always a good read. Here in particular section 3. Create a patch for each logical change. https://www.kernel.org/doc/Documentation/SubmittingPatches

Keep the diffs small. Seriously, try to keep the diffs small.

There is no need to change the order of punctuation_chars/casematch_flag.

If you extract a function try to keep the diff short so that one sees what changes. If you think the new function is in the wrong place afterwards, use a separate commit that just moves the code. That is easier to check.

If you extract from more than one source, it might be worthwhile to synchronize the sources in one commit and then deduplicate with the extraction where nothing changes otherwise.

The pattern

You are still using the pattern with the non-capturing parentheses. Are you sure that is necessary? I tried it without, just with the assertion, and it worked with r'(?<!%s)%s%s+'. The tests passed, too.

Style

Don't add lines longer than 80 characters. This plugin might be helpful, although it requires a bit of setup. Check out the example setup that replaces vim-excess-lines.

https://github.com/dirkwallenstein/vim-match-control

[aaronjensen]

Here you go, give it a look over please.

[dirkwallenstein]

Looks good. I give it a full test tomorrow. You could fixup the last commit by breaking the long line and removing the obsolete comment above that line.

[dirkwallenstein]

Ups, my bad. No long line present. But if you like you could remove the obsolete comment

[dirkwallenstein]

There are long lines and whitespace errors (empty line at the end-of-file) in the test file.

Tests need to be refactored with the source code. Right now the debug-code is tested twice. It should be depulicated with the extraction.

Each function has its own unit-test-class. When extracting the function the tests need to be deduplicated, too. Now the new function is tested twice through both client functions. Unit tests isolate (as long as reasonable) the function under test. So there has to be a new unit-test-class for the new function and the test-case-classes for the client functions would become almost empty.

Other than that, it looks good.

[aaronjensen]

The current localcomplete tests are two tests in one, they test the haystack given as is and they test it split into multiple lines. This differs from the all buffer tests which just test a single line or multiple explicitly. Does it seem reasonable to just test multiple lines explicitly instead of implicitly in each other test case?

[aaronjensen]

The tests have been DRY'd up: https://github.com/aaronjensen/vim-localcomplete/commit/0e89f44fc0764951fbbfda3f539703d75f69e1d1

The complete tests are a little bulky now since I didn't extract a helper method for them. Since there was only one I thought that'd be ok for now.

[dirkwallenstein]

Thanks. Applied with whitespace fix.