Detect original filetype of an embedded puppet file #146
Conversation
|
thanks for working on this @shadowwa ! it was something that I wanted to get to for a while I've tried the changes in this PR and so far the results are:
should number 2 have figured out the type via the file name? |
|
Strange, I tried with a minimal .vimrc to be sure that I haven't any other plugin or configuration interfering call plug#begin()
Plug 'shadowwa/vim-puppet', { 'branch': 'detect_original_filetype','for': ['puppet', 'epuppet'] }
call plug#end()and it's working fine
i initially used vim 9.0 but I have the same behaviour with vim 9.1 and neovim 0.7.2 and neovim 0.9.5 |
|
hmm strange.. I've commented out everything in my .vimrc and kept just the plugin activation like in your example, and disabled all other loaded files except for plug.vim and I'm still seeing the same thing. when I do I'm using vim 9.1 in debian sid. I'll try and do some tracing of the filetype detection to see if I can understand what's happening |
|
err ok something's off on my side. I cloned your repo and switched to the branch for this PR and now it's working as expected. I'll see if I have some weird state in my clone of rodjek's repository |
|
ah! I found something out. vim 9.1 has something that was merged upstream and it's already defining the 'epuppet' filetype detection: so the issue is not with your code but with this file short-circuiting things |
|
@shadowwa I took a peek at how subtype detection is currently done in upstream vim for eruby and I was able to replicate what it does for epuppet, squashing the bug that I have in vim 9.1 Basically the difference is that subtype detection is done inside I've prepared a branch in my own fork that builds on top of your branch to get the final result that works for me in 9.1: https://github.com/lelutin/vim-puppet/tree/detect_epuppet_subtype can I ask you to test if that works well for you as well in 9.0 ? if it does, we'll only need to add some unit tests for the new subtype detection and then we can merge this feature to master in this repo |
|
The first version of the patch I wrote was also base on the eruby.vim but I found out that it was only taking into account the file extension and not the path or shebang as vim would do for files without .epp. I tried to reproduce your error with my patch by using a fresh installed version of bookworm upgraded to sid but didn't succeeded. here are the diffents result I got:
Footnotes
|
|
huh interesting, thanks for doing all of those tests on many different releases. it's kind of a shame how much effort we must put on the github CI in order to test in other releases I |
|
Gah.. I hit something by mistake while typing. Didn't mean to close this. I was going to say: I did some additional testing just now and somehow now I'm not getting So your code does a better job at detecting than mine, but the detection is flaky. I'm currently out of ideas for where to direct things. Do you have an idea for how we could make detection in your code more reliable, given the presence of filetype detection in upstream vim? |
|
while trying to complete vader tests, I saw that (even on the rodjek/vim-puppet master branch) all tests for file detection using failed when using vim from latest debian, ubuntu or mageia (and neovim 9.5). Did you also noticed a failure of these tests on the rodjek/vim-puppet master branch? Using vim appimage, I was able to see that while version 9.0.0260 was working fine, 9.0.0270 broke the tests. I've created another branch to test and it seems to work for vim above 9.0.0270 and with neovim 9.5 |
|
I've found another reason explaining the "flakiness" of our results: I assumed that loading the plugin by modifying rtp (as done in test/init.vim) or using plug-vim would be the same, but I was wrong:
To solve the problem, I just have to do the same thing that is done for .pp file: use Now the subfiletype is correctly detected on vim and nvim (at least up to v0.7.2 from mantic) and even solve the slow starting and epuppet.epuppet I got on vim native package of mantic. |
shadowwa
force-pushed
the
detect_original_filetype
branch
from
96d1b8b to
23f83ce
Compare
from neovim 0.8.0, filetype detection is done with filetype.lua instead of filetype.vim. The autocommand added in ftdetect/puppet.vim will always come after the lua detection from nvim and while working fine for simple filetype assignement, it does not for the subfiletype detection. As I did not found a way to remove a filetype from lua detection, I had to replace it with the same logic that was done in the vimscript file. The autocommand has to be disable to not interfere with the lua detectection.
shadowwa
force-pushed
the
detect_original_filetype
branch
from
23f83ce to
e5ac7cb
Compare
|
I've finally solved the filetype detection for neovim >= 0.8.0. |
|
Hello @shadowwa wow! thanks for pushing this forward. I really appreciate the work that you put into this (also it's definitely a feature that I want in this module). (this is odd.. why didn't github trigger the CI on your latest pushes?..) trying a new test run with your changes:
|
|
small parenthesis: I've just tried reactivating neovim tests in CI and they don't cause coredumps anymore! 🎉 so we can have neovim tests in CI again. interestingly, some tests are currently failing on neovim on master and this branch is fixing them! see: https://github.com/rodjek/vim-puppet/actions/runs/10589033615/job/29342493381?pr=147 I can reproduce the neovim fails locally with nvim 0.9.5 so your branch is making some part of the code better. we just need to figure out what's happening with vim 9.1 and sub-type detection |
| @@ -11,5 +11,14 @@ au! BufRead,BufNewFile *.pp setfiletype puppet | |||
| " Some epp files may get marked as "mason" type before this script is reached. | |||
| " Vim's own scripts.vim forces the type if it detects a `<%` at the start of | |||
| " the file. All files ending in .epp should be epuppet | |||
| au BufRead,BufNewFile *.epp setl ft=epuppet | |||
| if !has('nvim-0.8.0') | |||
There was a problem hiding this comment.
now that there's a .lua file, does this need to specifically exclude nvim 0.8.0+ ? e.g. will nvim versions prior to 0.8.0 not be able to use the lua script? (asking out of total ignorance about this)
There was a problem hiding this comment.
I don't know, I can't remember if I've tested older nvim without this 'if'.
|
ok this is not super useful as a "how do we fix this" but I can confirm that the test failures on vim are caused by this line: https://github.com/vim/vim/blob/master/runtime/filetype.vim#L1847 If I comment it out, the tests all pass with vim 9.1 (current package on debian sid). to see a bit more what was happening, I've added |
|
@shadowwa I did a bit more digging around and here's what I can find:
So the code proposed in this PR is novel and we should find a way to make it work! If we want to think about the future though, I think that we should work with both upstreams, vim and neovim, to get the subtype detection for note: I'm not willing to wait for the above to happen before we merge your code in. I think we should get your work finalized and merged into the vim-puppet plugin, and then work on pushing the changes to the upstreams. If we're able to get the change merged into the upstreams, then we could add a version fence around filetype detection in this plugin to let the builtin type detection happen starting with the versions where the work got merged in. Then in a couple years once we decide to stop supporting versions of vim and neovim older than those points, we'll be able to trop the filetype detection code from this module. Do you think this idea is worth moving towards? Would you like to open issues and/or pull requests on neovim and eventually vim ? or would you prefer to let me handle this? I don't mind opening issues, but I want to leave the you the option of being the point of contact in vim/neovim wrt the changes to you if you so desire. |
|
@shadowwa it seems as though the line if I add see: https://vimhelp.org/options.txt.html#%3Asetf unfortunately, I have not found a way to "unset" filetype in order to let the setf do its thing. I've reworked the code for this merge request and yet again moved closer to how things are done for eruby. It only detects the subtype by the filename's extension but not via the shebang or the path to the file. This PR has been open for a long time now and the state of my rework is better than nothing. I'll open up an issue and tag you there so that we can follow up on the missing bits: reusing builtin filetype detection to get content parsing and discovery through the path to files. |
|
Testing the latest version shows that if I got the highlight correctly detected based on the subtype file extension before the .epp, I'm losing other niceties like linting of the subtype by ALE or activation of the subtype LSP by coc.nvim. let &filetype=b:epuppet_subtype .'.epuppet'in syntax/epuppet.vim does the trick but as you mentioned in 4938353, setting the filetype in syntax file should not be the way. Weirdly, I was using my branch almost every days with several vim version, and the detection was always OK. And for nvim, I didn't saw that it was updated to 0.10.1 and testing now show that my branch didn't work anymore. I don't really get why we have opposite results regarding vim vs. nvim. I'll try to see if I can find a way to get the coc/ale works with highlight while setting the filetype on the ftdetect place, and why we get opposite result with vim/nvim. A working subtype detection should really be nice on upstream vim/nvim. |
vim-puppet is currently loading syntax/sh.vim and adding specific puppet region for highlighting.
I tried to detect more finely the original type by asking vim to do a filetypedetect on a filename without 'epp' extension.
I also changed the &filetype value to originalfiletype.epuppet so various plugins like ALE or coc.nvim recognise the original filetype and allow linting or autocompletion
I tried yaml, toml, various conf file and didn't find any drawback.