pearofducks / ansible-vim

A vim plugin for syntax highlighting Ansible's common filetypes
MIT License
800 stars 98 forks source link

Use setfiletype to allow overrides #54

Closed maxwell-k closed 6 years ago

maxwell-k commented 6 years ago

Ansible users can choose which syntax to use for files. For example a user could write a static inventory file - hosts - in INI or YAML syntax [1].

:help setfiletype explains that setfiletype will:

Set the 'filetype' option to {filetype}, but only if not done yet

This allows the user to write their own ftdetect autocommands to choose a different file type from the default.

Without this change ftdetect auto commands from this package are read much later in vim's startup process and therefore take precedence over the user's own. Without this change, this behaviour is difficult to work around, for example it is very difficult to write ftdetect autocommands that set the hosts file syntax to YAML.

[1] http://docs.ansible.com/ansible/latest/intro_inventory.html#inventory

pearofducks commented 6 years ago

Hey! Sorry for the delay here - I'll probably have time to look at this in the next few days.

Could you quickly clarify the use/benefit to users and the intended use? (Ideally in README).

Also if you could change the base branch to v2 that'd be awesome. :)

maxwell-k commented 6 years ago

I added the use to README and I'll try and explain the benefit here:

Ansible provides flexibility: it lets you choose between different file types, I use the example of the hosts file which can be YAML or INI-like, both are shown in the documentation.

Before this change anything you set in your vimrc is later overridden by the ftdetect/ansible.vim file from this package. The output from :scriptnames is one way to see this:

  1: ~/.vim/vimrc
  2: /usr/share/vim/vim80/ftoff.vim
  3: /usr/share/vim/vim80/filetype.vim
 ✃
 16: /usr/share/vim/vim80/syntax/syntax.vim
 17: /usr/share/vim/vim80/syntax/synload.vim
 18: /usr/share/vim/vim80/syntax/syncolor.vim
 ✃
 34: /usr/share/vim/vimfiles/pack/ac/start/ansible/ftdetect/ansible.vim
 ✃

This PR changes ftdetect/ansible.vim to use setfiletype instead of set filetype=. setfiletype only sets the filetype option if it hasn't already been set. This stops ftdetect/ansible.vim overriding your own choice from vimrc or elsewhere.

The commit making the change is only three lines added & deleted, the explanations have ended up being much longer.

pearofducks commented 6 years ago

Against a minimal vimrc, this (seems to) completely break ftdetection.

Is something missing in your changes? Or are you taking special actions in your own vimrc that could be working around this?

With the vimrc below, and this changeset, I get yaml as a filetype for files that should be detected - and no additions from this plugin.

set nocp
call plug#begin('~/.vim/plugged')
Plug 'pearofducks/ansible-vim'
call plug#end()
filetype plugin indent on
syntax on
set ai
set si
pearofducks commented 6 years ago

I'm closing this in favor of #57 which seems to accomplish the same end-result (letting users easily override ftdetect), but seems to be a more straightforward implementation and follow some established standards.

Thanks for your work here and hopefully this change fits your use-case as well!