rodjek / vim-puppet

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

embeddedpuppet files sometimes get detected as 'mason' instead #127

Closed lelutin closed 3 years ago

lelutin commented 3 years ago

I've hit some cases where type detection was off. One example is for embeddedpuppet where the default types from core vim were taking over.

A simple reproducible example of this is to create a file that starts with a <% tag:

cat > blah.epp <<EOF
<% if $yah { %>
blah
<% } %>
EOF
vim blah.epp  # this file gets detected as 'mason' because of the opening tag.

According to vim's documentation, the default type detection happens before the ftdetect scripts, so they can sabotage plugins since setf won't run if a type was already detected (why is it parsed in this order? blah) and plugins can override the type if needed by using setlocal ft=... instead of setf.

http://vimdoc.sourceforge.net/htmldoc/filetype.html#ftdetect

So for embedded puppet, we can probably safely override type detection for all *.epp files since this extension is not present in default types.

However for \*.pp files it's more delicate: this extension is already mapped to the pascal type in the default detected types.

see: /usr/share/vim/vim82/filetype.vim

Type detection currently somehow works for .pp files since we're using au! which removes all autocommands for BufRead and NewFile events liked to *.pp before it installs our very own command (see :help autocmd-remove). This means that the plugin is completely overshadowing the pascal file type for files ending with .pp just by being in the runtimepath.

we could forcefully set the file type only for files matching */manifests/*.pp but that makes the pascal type reappear unexpectedly when we just play around with single files.

overshadowing pascal type detection may be an acceptable outcome for the plugin's users and I wouldn't know how to do otherwise.

dkearns commented 3 years ago

I just, coincidentally, attempted a fix yesterday to distinguish between Pascal and Puppet for .pp files. This was committed in revision https://github.com/vim/vim/commit/a0122dcd1cc9e9bb62c071a9b91426a8bce4f8d9

It works by inspecting the file for Pascal syntax and falls back to Puppet if none is found.

Hopefully that solves some of your problems here?

lelutin commented 3 years ago

@dkearns oh yes that sounds really interesting, and a better option than to just forcefully overshadow Pascal files. I couldn't find the commit or the repository in your account though. Could you point me to a place where I could find this commit?

also, do you happen to know how we could add a test for filetype detection? I've looked up what vader.vim can do and I get the feeling that testing ftdetect is not really feasible...

dkearns commented 3 years ago

Sorry, I've updated the commit link.

I'm not familiar with Vader but I just tried the following and it seems to work?

Execute:
  edit foo.pp
  AssertEqual &filetype, 'puppet'
lelutin commented 3 years ago

ah nice thanks for the change in vim :+1:

silly me, yep that works. I currently can't get it to open up a real file containing the bogus leading tag for embeddedpuppet though but I'm sure I can figure something out.

lelutin commented 3 years ago

I just pushed a test to #128 for filetype detection of embeddedpuppet. I'll wait for some amount of time to see if some folks give me feedback on the PR.

dkearns commented 3 years ago

I'm was intending to send a patch upstream to add detection for Puppetfile and *.epp files.

Is there a reason to name the EPP filetype embeddedpuppet rather than epuppet which would match the ERB filetype of eruby?

lelutin commented 3 years ago

not really any good reason no. epuppet does sound better and makes the similarity to eruby more evident, so I'm willing to rename the filetype

dkearns commented 3 years ago

Thanks. I updated the filetype detection in Vim a couple of days ago as well.

https://github.com/vim/vim/commit/8323cab31c3120a7f80cf3271a506a30ec04d99e#diff-c07c0771a9f5af08e375703487a8fe7bbddd71f98a7bbe06acd78365525a243e

lelutin commented 3 years ago

nice, this way this plugin and upstream vim will agree on the name (if only I could get the tests to work for filetype detection though :)