Shougo / neocomplete.vim

Next generation completion framework after neocomplcache
2.74k stars 203 forks source link

Default CSS and SCSS Omni completion input patterns allow too-short strings #604

Closed jsit closed 6 years ago

jsit commented 6 years ago

Omni completion starts in CSS and SCSS files when a single character is typed, which is too aggressive in my opinion. Ideally, I think this should obey the g:neocomplete#min_keyword_length setting.

Here's the current pattern:

https://github.com/Shougo/neocomplete.vim/blob/4cf290ec3202ad5cda6e4a15b0106f1c2feb2927/autoload/neocomplete/sources/omni.vim#L53-L56

I think it should be this:

call neocomplete#util#set_default_dictionary(
      \'g:neocomplete#sources#omni#input_patterns',
      \'css,scss,sass',
      \'\w\{' . g:neocomplete#min_keyword_length . '\}\|\w\+[):;]\s*\w*\|[@!]')

This would require a minimum of X word characters before calling Omni completion.

This proposed change also requires a ), :, or ; character between one string of word characters and the next before trying Omni complete on the second string (but does not require a whitespace after those characters).

Shougo commented 6 years ago

This proposed change also requires a ), :, or ; character between one string of word characters and the next before trying Omni complete on the second string (but does not require a whitespace after those characters).

Please describe the change. Why it is needed? I don't understand it.

jsit commented 6 years ago

I guess I don't get what that second string is meant to do. This regex string:

\w\+[):;]\?\s\+\w*

Is (a) at least one word character, followed by (b) one or none of the ), :, or ; characters, followed by (c) at least one character of whitespace, followed by (d) at least 0 word characters.

It looks like this is supposed to be for when you have something like background: n, there is supposed to be Omni completion for that n; but because the [):;] is optional, it's activated for the first letter of any word that has another word before it. So it will activate for all the following strings:

div a background: n border: 1 /* Lorem i color: red; b

What's odd, though, is that the space following [):;] is not optional, so Omni would not trigger on something like this:

color:r

I think I just don't get what this second string is meant to target that the first string isn't already targeting. Looking more closely at it, it seems like you could almost get rid of it, except maybe after a : character so that property values pop up earlier.

Shougo commented 6 years ago

Well, I get it.

But it is not too agressive?

Omni completion starts in CSS and SCSS files when a single character is typed, which is too aggressive in my opinion. Ideally, I think this should obey the g:neocomplete#min_keyword_length setting.

Please describe why \w\+ is too agressive, but \w\+[):;]\s*\w* is not.

jsit commented 6 years ago

Please describe why \w\+ is too agressive, but \w\+[):;]\s*\w* is not.

I think because, at least with :, you know the user is starting a property value, of which there are limited options. But maybe it should be tied to g:neocomplete#min_keyword_length, too. Why even have that second string (\w\+[):;]\s*\w*)? What are some example use cases for it?

Shougo commented 6 years ago

I don't write CSS. The pattern was added long ago by other people.

So I need CSS writer's demand.