rodjek / vim-puppet

Puppet niceties for your Vim setup
Apache License 2.0
500 stars 137 forks source link

added set option to correctly set iskeywords #137

Closed lcrownover closed 2 years ago

lcrownover commented 3 years ago

This PR fixes #108

lelutin commented 3 years ago

Hi there, thanks for pushing for this change too. The more confirmation that it's annoying ppl the more confident we are with moving back.

I'm wondering though if this method of assigning the iskeyword variable, with -= instead of = has a special significance. There are still many things from vimscript that I haven't fully explored yet.

I've tried to echo the contents of iskeyword before and after your patch, and also with the setlocal line commented out and in both of the latter cases the value was the same (e.g. it's the default value for non-win32 platforms according to the help text for iskeyword).

lcrownover commented 3 years ago

After doing some testing and reading of the vim help docs, I'm not sure of the original intention with the current line:

setlocal iskeyword=:,@,48-57,_,192-255

It's my understanding that this line sets the colon and underscore characters as keywords, which means they will not act as separators.

The default iskeyword value on a vim --clean shows (:set iskeyword?):

iskeyword=@,48-57,_,192-255

The @ represents any characters where isalpha() is true.

If the character is '@', all characters where isalpha() returns TRUE
          are included.  Normally these are the characters a to z and A to Z,
          plus accented characters.  To include '@' itself use "@-@".  Examples:
                  "@,^a-z"        All alphabetic characters, excluding lower
                                  case ASCII letters.
                  "a-z,A-Z,@-@"   All letters plus the '@' character.

By default, the colon character is not a keyword, which is what we want. With the popularity of snakecase, I'm also in favor of doing the common modification of removing the underscore from the keywords, hence the `-=` in the set command.

I can also see the benefits of hard-coding the keywords, as I had in my first commit:

iskeyword=@,48-57,192-255

As that would override any previously set keywords with the defaults, with the exception of the underscore.

Notice the difference between setting the value with =, or modifying the value with -=

However, for the sake of allowing users to keep their custom keywords, I'd still prefer the relative configuration of simply removing the underscore.

lelutin commented 3 years ago

I believe that the intention of adding : in iskeyword was to make whole resource names into valid keywords, although I can't venture much more than this guess since the git history doesn't quite explain the reasoning behind the change.

However, in the above intentioned case only the sequence :: should be considered for keywords. But because of how iskeyword functions we can't represent that and only that with that option.

Furthermore, the colon character is also used within the puppet DSL in places where it is not a part of a keyword, so it is not a great idea to add that character in iskeyword.

Now why was this option set with a hardcoded value instead of modifying the default value to add the required parts, I really can't tell... Now that we look at this some years after it was pushed in, it seems as though it wasn't the right approach.

As for removing the _ character, I would tend to disagree with this. It is a character that's contained in the iskeyword option by default, and according to Puppet's upstream documentation0 it is a valid character for a keyword in the DSL.

Because of all of the above, I would tend to simply remove the entire setlocal line for iskeyword and add in its stead a comment that explains why we don't want to include the colon character in there.

Then if you desire to remove the underscore, you could add an ftplugin/puppet.vim file in your own vim directory to remove the underscore from the option in your own setup.

lcrownover commented 3 years ago

Sounds good to me. I've removed the line altogether, effectively removing : from iskeyword, which should now make :: separate word motions.

Looking at the docs, I can agree that the default behavior should include _ in iskeyword (default).

lcrownover commented 2 years ago

Just checking in on this. Is there anything I can do to make this better?

lelutin commented 2 years ago

thanks for your follow up!