rodjek / vim-puppet

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

Override prior type detection for embedded puppet (closes: #127) #128

Closed lelutin closed 3 years ago

lelutin commented 3 years ago

Currently, if an .epp file starts with a code opening tag, <%, it gets auto-detected as being of the mason type by Vim's core type detection, which runs before ftdetect scripts are interpreted. The .epp extension is not used by anything else, so we want to make sure that the core autodetection doesn't take effect here.

To also document the other exception to file type detection in our plugin, the use of au! is necessary for .pp file since the core type detection already assigns them as pascal. Let's make a note as to why we're using this forceful command in this particular case.

The effect of the ! after the au keyword is that all autocommands get removed and then this one gets added. This could potentially hide some other autocommands that would be supplied by other plugins or user scripts that get loaded before this plugin.

There should not be a need for an override of any type for Puppetfile, so let's stop using the ! suffix for that case.

lelutin commented 3 years ago

this iteration works better than my previous one. it solves the issue with .epp files and clarifies why we have those overrides in filetype detection

lelutin commented 3 years ago

I've rebased this on top of master now that the nvim tests are fixed.

lelutin commented 3 years ago

I've looked over in vader.vim and trawled through a couple of the projects in vader's wiki page about which projects are using it and I don't think that we can test filetype detection using vader unfortunately. the content in the Given blocks are not passed through file type detection and we're testing without a file with an predictable name.

lelutin commented 3 years ago

@rodjek do you have an idea of how we could test filetype detection for puppet and embeddedpuppet files?

lelutin commented 3 years ago

ok now we have a test case for this. I'm not super happy with the test since it creates and then deletes a file in the git repository but I don't think vader has the means to help us to do better than this :\

I'll wait for a tiny bit of time to get feedback on the PR.

lelutin commented 3 years ago

I've integrated the fix to the test you suggested and rebased on top of master so that I could reverse the order of commits: first add a test, then fix the problem.

I think with this bit of change we should probably be ready to merge this in, provided that CI is happy with the current state.

lelutin commented 3 years ago

with that trick you showed me, I couldn't help but add simple filetype detection tests for .pp and Puppetfile files too, for good measure.

lelutin commented 3 years ago

ah interesting .. I thought I was going mad since the tests were failing for every commit on this branch when I was running them locally -- even though the filetype detection is working correctly -- so I pushed to see if travis would obtain the same results as I was.

I currently don't really understand why the tests are failing though.. When editing files normally, detection does work correctly. Also when I implemented the tests as suggested with filetype detect, I remember that they were working locally.

lelutin commented 3 years ago

ah .. one interesting bit of clue came up when I tried something: if I edit the Puppetfile of a control repository, it gets detected as ruby.

but then if I run the following, the filetype reverts back to conf !

:filteype detect

so now that I'm able to mimic the error in the tests, I'll have to figure out why it's doing something different than when vim loads up or opens up a Puppetfile for edition after startup (in both cases the file gets detected correctly) and unbreak the tests

lelutin commented 3 years ago

I've amended the last commit since vim's documentation says that files in ftdetect don't need to declare an augroup. we should rather disable the vint check for that particular condition in this file.

it does not fix the tests, but hopefully I'll end up finding out how to get there soon

lelutin commented 3 years ago

apparently for Puppetfile, if there is a comment line at the start of the file, filetype detect intercepts detection and the filetype becomes conf ... again with vim's default type detection that comes in before plugins gah why ..

but also.. why is this not happening consitently when editing a file with a comment line at the start?

also if I remove the comment line at the start once I'm stuck in conf filetype and if I run again :filetype detect, vim doesn't go back to the ruby filetype. this is getting annoyingly inconsistent

lelutin commented 3 years ago

exactly the same thing happens with epuppet and mason. the type detection in vim's scripts.vim takes over completely

lelutin commented 3 years ago

ok I found out why calling :filetype detect does not do what we hope it to do: when we call it manually, we're not triggering any of the autocmd actions BufRead or BufNewFile so the autocmd is not processed and we only get vim's built-in type detection.

Now with that info, I need to think of a way to get the tests to trigger those actions so that detection can be tested correctly.

lelutin commented 3 years ago

and I found it! with :edit without any argument we can trigger autodetection through the expected actions.

oof. I'll just wait for CI to confirm the fix and with that I think the patch series is complete and ready for merge