Various Enhancements #2

Open
wants to merge 8 commits into from

3 participants

@inkarkat

Hello thinca-san,

I have been using your excellent plugin for quite some time, as I found it to be the best among various "local configuration" plugins, especially because it allows both global and filetype-specific configuration.

Using the latter, though, I found some inconsistencies, especially in the order of sourcing, that were frustrating my attempts to define global configuration in a .local.vimrc and then override some of that in a filetype-specific localrc.

I have extended the plugin logic to deal with this in the correct order (which is somewhat complex, necessitating a hook into Vim's ftplugin.vim script), but now it works satisfactorily for all my advanced uses.

Please have a look at my changes, maybe try them out yourself, and feel free to incorporate some or all of them into your official plugin version, so that other users can benefit from my extensions, too.

@thinca thinca commented on the diff Jun 8, 2012
autoload/localrc.vim
@@ -7,13 +7,47 @@
let s:save_cpo = &cpo
set cpo&vim
+function! localrc#loadft(templnames, ft)
+ if empty(a:ft)
+ return
+ endif
+
+ " The 'filetype' setting can consist of multiple filetypes, separated by a
+ " dot, e.g. "foo.bar". Settings for subsequent filetypes override earlier
+ " ones, so execute filetype-specific localrc files in this order:
+ " foo, bar, foo.bar
+ let ftparts = split(a:ft, '\.')
+ for combination in range(1, len(ftparts) - 1)
+ call add(ftparts, join(ftparts[0:combination], '.'))
+ endfor
@thinca
Owner
thinca added a note Jun 8, 2012

Vim's ftplugin system doesn't load filetype plugin named "foo.bar"(ftplugin/foo.bar.vim).
Why this includes "foo.bar"?

@inkarkat
inkarkat added a note Jun 8, 2012

Right, but I may want to override settings only when the combined filetype is "foo.bar", not just "foo". This is just a convenience. Otherwise, I would have to install a local config for "foo", and in there check for &filetype ==# 'foo.bar'.

@thinca
Owner
thinca added a note Jun 8, 2012

OK, I understood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@thinca thinca commented on the diff Jun 8, 2012
autoload/localrc.vim
function! localrc#load(fnames, ...)
for file in localrc#search(a:fnames,
\ 1 <= a:0 ? a:1 : expand('%:p:h'),
\ 2 <= a:0 ? a:2 : -1)
- " XXX: Handle error?
- source `=file`
+ try
+ execute 'source' fnameescape(file)
@thinca
Owner
thinca added a note Jun 8, 2012

Why you changed the sourcing way?
I think fnameescape() has problem.
For example, :edit fnameescape('bang!.txt') opens !.txt on bang/ directory on MS Windows.
Because, fnameescape('bang!.txt') makes bang\!.txt.

@inkarkat
inkarkat added a note Jun 8, 2012

I addressed your TODO; when there are syntax errors in the local config, it printed ugly multi-line exceptions. Now, only the actual error message and the context (i.e. sourced file) are printed.

fnameescape() was introduced for exactly this; unfortunately, it has a bug on Windows; I have already reported this: http://groups.google.com/group/vim_dev/msg/0b616f0cfe00c909

@thinca
Owner
thinca added a note Jun 8, 2012

Error handling is OK, thanks.
Why you changed source `=file` into execute 'source' fnameescape(file)? Is source `=file` wrong?

@mattn
mattn added a note Jun 8, 2012

I guess that this's not answer for thinca's question.
Why you change =file to fnamemodify? =file seems to be working good to me.

:so `="foo!bar.vim"`
@inkarkat
inkarkat added a note Jun 8, 2012

I actually had a strange interaction with the former: 'wildignore' is applied to this backtick-expansion, so it can have unwanted side effects ("E480: No match: `=file`"), so prefer fnameescape() in plugins, and just use backtick-expansion interactively in Vim.

In my case, I disabled a local filetype-specific configuration by appending .bak, but that one still matched my g:localrc_filetype, then caused the E480 error, because I have *.bak in 'wildignore'.

@mattn
mattn added a note Jun 8, 2012

Moved comment into this thread.

Please let me know the way to reproduce it. I don't think that your change is strange. I want to fix vim if it's a bug.
I'm understanding localrc#load don't treat wildcard. It take just real name. So I wonder why you are talking about wildignore.

@inkarkat
inkarkat added a note Jun 8, 2012

file wildcard != wildignore, and this is not a Vim bug, just a peculiarity of backtick-expansion.

If you have a local config named .local.foo.vimrc and :set wildignore=*.vimrc, this should trigger the error when editing a filetype foo. The filename gets passed into the function, but backtick-expansion, as it observes the 'wildignore' setting, fails to expand it, so the sourcing fails.

@inkarkat
inkarkat added a note Jun 8, 2012

This shows the problem even without localrc:

:edit `="$HOME/.vimrc"`
" (opens .vimrc)
:set wildignore=*.vimrc
:edit `="$HOME/.vimrc"`
" E480: No match: `=/home/user/.vimrc`
@thinca
Owner
thinca added a note Jun 8, 2012

You right. I didn't know. Thanks!

@mattn
mattn added a note Jun 8, 2012

Then, it should do

let old_wildignore = &wildignore
set wildignore=
so `=file`
let &wildignore = old_wildignore

I think this is only way to avoid a bug of fnameescape(), and annoyed problem about escaping filenames.

@inkarkat
inkarkat added a note Jun 8, 2012

@mattn: What's so bad about fnameescape()? It was created explicitly for this use case; I use it in almost all of my plugins. The bug with ! isn't that serious (would be great if you could actually fix it, though :-)

Your alternative is long and cumbersome, and you have to put the last line into a finally clause to be sure that it gets executed when the sourcing causes errors.

@mattn
mattn added a note Jun 8, 2012
@inkarkat
inkarkat added a note Jun 8, 2012

fnameescape has more bugs that can't really pass to the original argument to command(without !).

Have you raised this on the vim_dev mailing list? If there are really serious issues, I think fixing the function is way better than some groups silently working around it. Maybe I'm not using that many strange file names, but for the most part this works for me just fine.

This is famous issue in japanese vim plugin developers.

You mean this group: https://github.com/vim-jp? This is interesting. Do you closely work together, or even know each other in person? (I'm German, but married to a Japanese woman, and been to Japan twice.) I was surprised how quickly you (@mattn) picked up my pull request, although it was for @thinca.

@thinca
Owner
thinca added a note Jun 8, 2012

This is hard problem...
I use fnameescape() for now.
If any problem is reported with this way, I'll switch to @mattn's way.
'wildignore' is a global option, so I think it has no major problem for this plugin.

@mattn
mattn added a note Jun 9, 2012

You mean this group: https://github.com/vim-jp?

Yes, but not all.

Do you closely work together, or even know each other in person?

No. :)
I'm just an user of vim-localrc. :)

(I'm German, but married to a Japanese woman, and been to Japan twice.)

Oh, you know japanese well :)

I was surprised how quickly you (@mattn) picked up my pull request, although it was for @thinca

I often talk to him about vim plugins. :)

vim-jp is working group that hope to make developer's network, and fix vim bugs, provide binary packages, writing blog entries, provide commonly plugins.
We discuss in japanese and send many patches to vim-dev.

http://vim-jp.org/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@thinca thinca commented on the diff Jun 8, 2012
after/ftplugin.vim
+runtime ftplugin.vim
+
+" Handle filetype-specific localrc configuration after Vim ftplugins.
+augroup filetypeplugin
+ autocmd FileType *
+ \ if s:has_localrc() |
+ \ call localrc#loadft(
+ \ type(g:localrc_filetype) == type([]) ? copy(g:localrc_filetype)
+ \ : [g:localrc_filetype],
+ \ expand("<amatch>")) |
+ \ endif
+augroup END
+
+
+let &cpo = s:save_cpo
+unlet s:save_cpo
@thinca
Owner
thinca added a note Jun 8, 2012

This file conflicts when user has a plugin which does some hack.
hmm, but I have no other idea...
I also want this feature...

@thinca
Owner
thinca added a note Apr 26, 2013

How is this problem considered?

Do you have any concrete examples, or is this purely hypothetical? Such a user would find out, hopefully complain here, and with luck there's a workaround on the other side. Worst case, the user has to live without this functionality. But I haven't so far seen anyone modifying this area (and I've looked at dozens of user configurations and plugins), and it is generally frowned upon, as it's part of the default Vim distribution.

@thinca
Owner
thinca added a note Apr 26, 2013

Do you have any concrete examples, or is this purely hypothetical?

You are just trying to place now.
If this file is placed, you and other developers will not be able to use this approach forever from now on.
This approach is evil. I want to avoid this way.

Sure, it's ugly, and you're right that this can only be used by one plugin, but I don't see any better way; neither do you. It's especially bad that we have to integrate before and after the original functionality, so submitting a patch to the original ftplugin.vim would be ugly and a special just for this plugin, not nice.

I'm pragmatic about this: I don't know of any other plugin that needs this hack, and if there were, and many users would want to use them both, we would surely find out, and then this momentum could be channeled into a patch for more integration points in the original ftplugin, and Bram would probably accept this based on the shown user needs. Both disputed plugins would incorporate the new integration point, and could in the future work together. But until that conflict actually happens, I'd rather spend my energy elsewhere, especially because we can only guess where exactly in ftplugin.vim a conflict would actually happen.

One compromise that just came to my mind would be to move the actual :autocmds from after/ftplugin.vim to autoload/localrc.vim, so that the former just becomes:

augroup clear
call localrc#BeforeAutocmds()
runtime ftplugin.vim
call localrc#AfterAutocmds()

And then include the same calls in plugin/localrc.vim, too, but only call them when they haven't been executed by after/ftplugin.vim yet. This way, after/ftplugin.vim would be optional, and could be delivered with another filename after/ftplugin.vim.disabled. Users that want the correct execution order would be instructed in the help to rename the file (and deal with the consequences).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@thinca
Owner

Thanks for your pull request!

I have some questions. I commented to diffs.
I want to include this pull request when the questions are solved.

@inkarkat

Hello thinca! I noticed you did some minor changes (VimEnter and nested); I've rebased my pull request. The way I see it, I had previously addressed all your questions. I've been using my modifications without problems in the past 11 months. Will you please consider it for inclusion?!

-- regards, ingo

@mackwic

🆙

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment