Don't change default C-n C-p behaviour #409

Closed
purpleP opened this Issue Jan 3, 2017 · 9 comments

Projects

None yet

2 participants

@purpleP
purpleP commented Jan 3, 2017 edited

It's been a somewhat a mystery to me why vim has two word completions mappings, namely C-n and C-p, but I've finally found a use for them. In python programming for example it's very common to pass keyword arguments like this

bar = 'bar'
baz = 'baz'
foo(bar=bar, baz=baz, some='foo')

So with vim you can make this easier by not having to repeat bar and baz by typing bar=<C-p> which would result in bar=bar and so on. One other example is opening/closing xml/html tags, but I'm not writing html this days and there are better ways to do that anyway, but still.

So that would be nice way of doing this things if your plugin wouldn't break default vim behaviour.

Expected

What is expected is that if no character is typed in insert mode yet and user presses any of the completions mappings (C-n, C-p, C-x...) you just let vim do it's thing.

I've found a bits of code responsible for this. It's call deoplete#mapping#_set_completeopt() in completion_begin function. I've played with it for a while and dumped deoplete context variable into json for three different cases: C-p pressed with no character typed, one character typed (no completions from deoplete yet, two characters typed (completions from deoplete showed up). Then I've made a diff of the three json structures and I've found some differences, but none of them would allow me to detect my usecase (no characters typed so far).

@Shougo Shougo added the wontfix label Jan 3, 2017
@Shougo
Owner
Shougo commented Jan 3, 2017

It conflicts the auto completion behavior.
I cannot support it.

#372

If you don't like the behavior, you should not use the plugin.

@Shougo Shougo closed this Jan 3, 2017
@purpleP
purpleP commented Jan 3, 2017 edited

@Shougo No it isn't if user specified that auto completion should show up only after one or more character have been typed and it is a default behaviour with your plugin. I don't see how that conflicts with using your plugin. It maybe conflicts with your implementation, but is it really impossible to change? This shouldn't be hard to do.

@Shougo
Owner
Shougo commented Jan 3, 2017

If you think it is easy, please create the fix PR.
I will test it.

@purpleP
purpleP commented Jan 3, 2017

@Shougo Well, it should be easy for the author (you). I would have to get familiar with your code to do that. What I've found so far is that you first changing completeopt and then in python code you're checking that context['complete_str'] is in interval [min_pattern_length, max_pattern_length]. I would have to do understand how do you make complete_str, but basically you just have to change this around. First you check that there's a completions, and only then change completeopt.

Can you comment whether this would break something in the way you plugin suppose to work?

@Shougo
Owner
Shougo commented Jan 3, 2017

I have tried sometimes and failed.
I don't think it is easy. But I may be wrong.
So, please create the fix PR if possible.

Related issues:
#79
#107

@Shougo
Owner
Shougo commented Jan 3, 2017 edited

deoplete is asynchronous completion, so the restore timing is nothing.
neocomplete is not. So it is possible.

@purpleP
purpleP commented Jan 3, 2017 edited

@Shougo I will certainly try to read your code and fix things (if possible), but not today. While I'm at it I'll do a little code review.
Here's your code.

pattern = '('+'|'.join(disabled_syntaxes)+')$'
if [x for x in context['syntax_names']
    if re.search(pattern, x)]:
     return 1
  • First of all. You shouldn't use 0 and 1 as True and False. You're not in C and not in vimscript. It makes code less readable. Reasoning is simple. Remember zen of python? Explicit better than implicit. If the value is of type boolean, say so explicitly, don't make people guess.
  • Second. If you're using regex pattern more than once compile it first p = re.compile(...) and then use p.search(...).
  • Third. Use generators when you don't need all sequence. Saves memory and saving memory saves allocations and allocations saves time.
  • PEP8 says that you should drop spaces before operator and operand if there is a lower priority operator in the expression. For example x = x*x + 1, but shouldn't otherwise for example x = x+1 is not a pythonic code and so is '('+'|'.join(diabled_syntaxes)+'$'.

Combining all that this code is better look like this.

pattern = re.compile('(' + '|'.join(disabled_syntaxes) + ')$')
if next(filter(p.search, context['syntax_names']), None):
    return True

Again. Use generators when you don't need all sequence. Maybe I'm don't know something, but why evaluate iterator to a list, just to iterate over it?
for source_name, source in list(self.itersource(context)):

And the worst part is that you don't seem to have test for any of these code. So I can't refactor it, until I'll write tests and to do that I'll have to understand the big picture.

@Shougo
Owner
Shougo commented Jan 4, 2017

Thank you for the code review!
Unfortunately, I'm Python beginner.
So, I have been helped by @tweekmonster and more people.

Second. If you're using regex pattern more than once compile it first p = re.compile(...) and then use p.search(...).

Hm. But the regex result is cached internally.
This is really needed?

And the worst part is that you don't seem to have test for any of these code. So I can't refactor it, until I'll write tests and to do that I'll have to understand the big picture.

The worst part is the tests. Yes.
Please add the tests. I needed your help.

@Shougo Shougo added a commit that referenced this issue Jan 4, 2017
@Shougo Apply #409 @purpleP's review 72962c0
@purpleP
purpleP commented Jan 5, 2017 edited

@Shougo About regex caching. I've read the documentation and while it is says

The compiled versions of the most recent patterns passed to re.compile() and the module-level matching functions are cached, so programs that use only a few regular expressions at a time needn’t worry about compiling regular expressions.

I remember benchmarking regex once and seeing that re.compile is increasing performance. I'm not sure what the benchmark and the usecase were, but here's a benchmark for you.

Surprisingly for me it shows that if length of random_data is big (10000) the endswith method starts getting slower than regex method. Which is probably because of the inefficiency of python for loop. Regex module is probably written in C or something like that and because of that it's faster.

Important thing though that no matter how large is random_data compiled regex is faster than non-compiled. And in this particular case you wouldn't benefit anything from using non-compiled regex anyway because the number of lines that you need in both cases is the same (if you're following PEP8 and it seems you do)

@Shougo Shougo added a commit that referenced this issue Jan 9, 2017
@Shougo Fix #409 #411 restore completeopt 6e529ab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment