WIP: File include

[lelutin]

This patchset is based off of #113

I've been trying to fix an annoying bug that affects users with textwidth set: when reaching the point where a line is long enough, puppet#format#Format gets called and the underlying command that's currently used flips the two last characters. I've detailed why this is happening in a comment in auto/puppet/format.vim.
I've so far tried a couple different approaches to fix this up and none worked.
I'm wondering if we'd be better off here deciding to disable this line-breaking function for the "i" and "R" modes so that lines don't get broken up as you type but would rather require users to use the gq command to format long lines correctly.
I'm wondering if others have better ideas about this bug though.

The other thing in this patchset that I've tried to do and mostly accomplished is to setup include and includeexpr so that users can go on jumping from all references to another class/defined type to the actual file defining it.
I've hit a roadblock while implementing this though: say that you have a line that reads contain ganeti::config, when typing the gf command while the cursor is on the word "ganeti", vim doesn't consider the whole class name and jumps to the file "init.pp" unexpectedly.
a simliar thing happens if you have more than one part to the class name. for example if you had include apache::vhost::php, typing gf while the cursor is on "php" would try to find the file php.pp instead of vhost/php.pp.
I'm not sure why the includeexpr is not getting more than one part of the class name.

[tequeter]

Basic tests with / and that pattern seem to work, that's puzzling. Did you confirm that puppet#include#IncludeExpr() was indeed receiving only part of the class name?

Some stuff I noticed while looking:

• You can probably simplify (\\f\|::) with \k, although that will match include foo::::bar too. \f seems wrong though, as it's for filename characters.
• class{" and Class[" are valid, so these class\\s+, Class\\s+, and )\\s+ should be ...\\s* IMO. It'll match the invalid include", but that doesn't seem too bad (vs. a more complex regexp).
• When you've finally cracked this nut, would you go as far as adding support for the function forms of include() and contain() as well, and require()?

[lelutin]

Did you confirm that puppet#include#IncludeExpr() was indeed receiving only part of the class name?

well.. not exactly precisely enough for my taste..

I used :checkpath! to see what's been recognized. I can currently see this, which indicates that it did match something:

--- Included files in path ---
manifests/package.pp
manifests/config.pp
manifests/service.pp

I tried using :breakadd func puppet#include#IncludeExpr but then that function's not being called when I call gf. It's probably being called when loading the file in the buffer.

when I call gf on a part of the class name, I can see that it's searching only for the word on which I'm standing since it errors out when it can't find something.

You can probably simplify (\f|::) with \k, although that will match include foo::::bar too. \f seems wrong though, as it's for filename characters.

oh huh.. I didn't think of it this way. tbh I took the \f from the ruby ftplugin file :P maybe I should use a regex that matches more closely what puppet permits as a class name.

class{" and Class[" are valid, so these class\s+, Class\s+, and )\s+ should be ...\s* IMO. It'll match the invalid include", but that doesn't seem too bad (vs. a more complex regexp).

oh, you're right! those should be \\s* instead. I'll think of a more clever way to include all possibilities (including what you mentioned below).

When you've finally cracked this nut, would you go as far as adding support for the function forms of include() and contain() as well, and require()?

ah, yes good catch! include and contain are indeed functions so it should be possible to use the form with parentheses. and I forgot to add require in there. I wonder if contain() is also a thing..

[lelutin]

OK, after setting the breakpoint I was able to debug puppet#include#IncludeExpr.

It's called once for every match to include= when I call :checkpath. during those calls, I can see that the a:fname parameter contains the full class name (e.g. ganeti::install etc).

the includeexpr function is also called when I use gf. during those calls the function's a:fname is set to only the part that's under the cursor (e.g. ganeti). So gf uses some other method to determine "what's under the cursor" it would seem.

[lelutin]

ah! ok I figured it out: according to the documentation for gf, it uses isfname to determine what the path under the cursor should be.

according to the doc:

                        If the file can't be found, 'includeexpr' is used to
                        modify the name and another attempt is done.

but at that point the "damage" is already done :\

so the solution would be to modify isfname to add : in there, but since the option is only global, there's no telling if doing this will break something else unrelated >:[

[tequeter]

In my understanding (but I'm no Vimscript expert), filetype plugins should indeed never mess with stuff that isn't local to their buffer. The user may have a second buffer open in the same Vim instance that deals with a completely different language and assumes a "sane" isfname setting.

I would be tempted to redefine and reimplement gf for the buffer, first opening <cfile> if it exists, otherwise searching with the include pattern from the current line to emulate the desired behavior. But again, no expert.

NB: include and co in function form can span multiple lines and start at some unknown point above the cursor.

contain(
  'module::a',
  'module::b',
)
-> contain('module::c')

A partial solution would be to also match classname-looking words in include, with no requirement for a preceding include, contain, class etc. However, this can't work for the init.pp without matching pretty much anything. The fragment below has a zero-width negative look-behind assertion to ignore $module::var and ${module::var} (or is some interesting line noise ;).

\v<[${]@1<!(::)?[a-z][a-z0-9_]*(::[a-z][a-z0-9_]*)+>

[lelutin]

during discussions on the vim irc channel, someone gave me an example of a hack to modify isfname when calling gf (in a buffer-local remapping of the gf command) and then setting it back to what it was.

this does work and I'm able to now open the right files when calling gf. but I'm still a bit uneasy with the way that it's achieving this.

I guess as you're saying the best solution for this so far would be to try and match the buffer's contents under the cursor and before it in order to recognize cases where we have a include.

I guess the gf function could also end up being more funky and implementing file "find and edit" for other things in puppet manifests such as file source params, template() and epp() params. but... let's start with opening up files that contain a class/define definition :)

I'll have to wait some time to implement the "gf" override, but at least we're starting to see a glimpse of a solution around the corner.

[lelutin]

I'm still not super sure where I left this off since it's been a while.. for now just to make this PR more relevent I've checked to see if there were things that I could rebase on the current master branch.

The issue with inverted characters while autoformating and breaking long lines with textwidth set has been addressed recently. That was two of the three only commits on this branch that were still diverging. So we're down to just one WIP commit to work with and only one actual goal for the PR -- much easier!

[lelutin]

I've just added a first version of a test for our include file discovery as I currently understand the scope of things. I've left out some weird ways to call the include/contain/require functions like: 'blah::moo'.include() which looks pretty gross (but is valid puppet dsl). the test also does not check how gf behaves, which we'll probably want to check at some point.

I've been working on this module for a while now and I'm not super pumped right now to continue hitting the regexp hammer until I have something that's working as desired.. so I'll leave things like this for now.

at least now we have the start of a test to work against. and the PR is in a state that can be continued on

note: the test does have an issue though: the files are all marked as NOT FOUND since the files actually don't exist on disk. we'll have to create empty files in the test's tempdir to get this working properly.