Slightly improved clang-format vim integration

Hello,

I've attached a slightly modified clang-format.py for vim integration. Feel free to commit it to the official svn in case you like it. I'd suggest the following commit message:

'''
clang-format: Use a Python module in vim with config via 'g:' globals

clang-format.py is changed to a Python module (therefore renamed to
'clang_format.py'), which is imported once into vim and then directly
called from vim mappings. Configuration now works via global vim
variables 'g:clang_format_binary' and 'g:clang_format_style'. Using
global variables is more flexible than hard-coded values in the Python
script. For example, the style can be changed on-the-fly in 'autocmd
FileType'.
'''

Best,
  Steffen Prohaska

clang_format.py (3.63 KB)

Could you maybe describe what’s different, and why?

– Sean Silva

Could you maybe describe what's different, and why?

I tried to describe and justify the differences in the suggested commit message. Maybe it is a bit short. But it's not obvious to me what is missing.

Do you have any specific questions?

  Steffen

What version of vim are you using? There seem to be a number of things that don’t work for me (I’m on Ubuntu Linux):

  • :help python-special-path isn’t available

  • vim.vars doesn’t exist

  • Putting the file in ~/.vim/pythonx doesn’t seem to work (ImportError: No module named clang_format)

Overall, I think that being able to configure the formatting behavior from e.g. autocmd FileType is a win, but this patch doesn’t seem to work for me.

– Sean Silva

What version of vim are you using? There seem to be a number of things that
don't work for me (I'm on Ubuntu Linux):

- `:help python-special-path` isn't available

This feature is only available in higher patch levels of vim 7.3

- `vim.vars` doesn't exist
- Putting the file in `~/.vim/pythonx` doesn't seem to work (`ImportError:
No module named clang_format`)

This feature is only available in higher patch levels of vim 7.3

Overall, I think that being able to configure the formatting behavior from
e.g. `autocmd FileType` is a win, but this patch doesn't seem to work for
me.

Steffen, I believe the idea of your patch is great. Unfortunately it has

1) You rely on features of very recent vim versions

As this is not a complicated plugin, there seems no reason to drop support for older vim versions.

2) You require python in the .vimrc file

I would prefer to not require python in the .vimrc, but have the plugin report an error when it is called without python support.

I believe you can solve both problems by creating a very slim dummy plugin, that is in charge of loading the python module and interacts with vim.

Cheers,
Tobias

Hello,

What version of vim are you using?

I'm using a recent MacVim, which is based on vim version 7.4.

There seem to be a number of things that don't work for me (I'm on Ubuntu Linux):

I've checked in the vim history when the necessary features were introduced:

- `vim.vars` doesn't exist

This has been introduced in version 7.3.911:

http://code.google.com/p/vim/source/detail?r=f1eab4f77a6fe4b77508d86a68a6681195806607

An alternative is to use vim.eval("g:...").

- `:help python-special-path` isn't available
- Putting the file in `~/.vim/pythonx` doesn't seem to work (`ImportError: No module named clang_format`)

The import search path has been introduced in version 7.3.1163:

http://code.google.com/p/vim/source/detail?r=70b1178dec7919120632cdeee6056e38108356a7

Overall, I think that being able to configure the formatting behavior from e.g. `autocmd FileType` is a win, but this patch doesn't seem to work for me.

I've attached a revised script that should work with much older vim versions. See suggested commit message for details:

'''
clang-format: Use a Python module in vim with config via 'g:' globals

clang-format.py is changed to a Python module (therefore renamed to
'clang_format.py'), which is imported once into vim and then directly
called from vim mappings. Configuration now works via global vim
variables 'g:clang_format_binary' and 'g:clang_format_style'. Using
global variables is more flexible than hard-coded values in the Python
script. For example, the style can be changed on-the-fly in 'autocmd
FileType'.

The implementation should work with vim 7.1 and later (maybe even 7.0).
It has been tested with 7.2.330 (Ubuntu 10.04), 7.3.429 (Ubuntu 12.04),
and MacVim 7.4.

Note also that unnamed buffers are now properly handled. Previously,
a Python exception was raised for unnamed buffers by subprocess.Popen,
because it was called with None for vim.current.buffer.name.
'''

clang_format.py (4.4 KB)

Steffen, I believe the idea of your patch is great. Unfortunately it has from my point two issues:

1) You rely on features of very recent vim versions

I've addressed this in in the revised script that I've just sent in a separate mail.

As this is not a complicated plugin, there seems no reason to drop support for older vim versions.

2) You require python in the .vimrc file

I would prefer to not require python in the .vimrc, but have the plugin report an error when it is called without python support.

Python has also been required by the previous version of the script. So my proposed changes don't make the situation worse.

I agree that a full plugin would be ideal. I think that it would ideally be hosted in a separate git repo on Github, so that Pathogen or Vundle can be used to install it. However, I don't plan to create such a plugin anytime soon.

  Steffen

I think this generally seems like a good idea. However, I’d like to also keep the old version at least for a while for compatibility and because I think it is slightly easier to install until it can be used by Vundle etc.

That should however not keep us from also committing this. Either to some Github repository so it can be used by Vundle or to a separate directory inside cfe, e.g. tools/clang-format/vim. The current state where several editor integrations are just files in tools/clang-format is a mess anyway …

Steffen, I believe the idea of your patch is great. Unfortunately it has from my point two issues:

1) You rely on features of very recent vim versions

I've addressed this in in the revised script that I've just sent in a separate mail.

Great. Thanks for fixing this so quickly.

As this is not a complicated plugin, there seems no reason to drop support for older vim versions.

2) You require python in the .vimrc file

I would prefer to not require python in the .vimrc, but have the plugin report an error when it is called without python support.

Python has also been required by the previous version of the script. So my proposed changes don't make the situation worse.

In case python is not supported, the previous script only failed when pressing the formatting key combination. The new script will fail as soon as vim is opened and the configuration is parsed. I believe this is a regression for people who would like to use the same .vimrc file across different systems.

I am personally not one of those people, so if others agree this is not important, I will rely on their judgement. On the other side, I don't feel comfortable OKing or commiting the patch myself.

In any case, thanks for working on this.

Cheers,
Tobias

I think this generally seems like a good idea. However, I'd like to also
keep the old version at least for a while for compatibility and because I
think it is slightly easier to install until it can be used by Vundle etc.

That should however not keep us from also committing this.

Really? It seems like a good thing to ask for...

Either to some Github repository

Please don't... =[ I don't think we want more repositories holding this
stuff.

The current state where several editor integrations are just files in
tools/clang-format is a mess anyway ..

Yea, this might be a good motivator to figure this out effectively.

(FWIW you don’t have to rename the script, you could use `clang_format = import(“clang-format”))

Thanks for the comments.

I've attached v3 of the script that hopefully addresses all your comments (see inline replies below). Suggested commit message:

'''
clang-format (vim): Support config via 'g:' globals, importing as Python module

'clang-format.py' is changed such that it can either be executed
directly as before or imported and used as a Python module. Both
alternatives are described in the introductory comment in the script.
Since the natural way of importing Python modules ('import <module>')
only works if the module name does not contain dashes, the file is
renamed to 'clang_format.py'.

Configuration now works via global vim variables 'g:clang_format_binary'
and 'g:clang_format_style'. Using global vim variables is more flexible
than hard-coded values in the Python script. For example, the style can
be changed from vim on-the-fly without modifying the Python script.

The implementation should work with vim 7.1 and later (maybe even 7.0).
It has been tested with 7.2.330 (Ubuntu 10.04), 7.3.429 (Ubuntu 12.04),
and MacVim 7.4.

Note also that unnamed buffers are now properly handled. Previously,
a Python exception was raised for unnamed buffers by subprocess.Popen,
because it was called with None for vim.current.buffer.name.
'''

Python has also been required by the previous version of the script. So my proposed changes don't make the situation worse.

In case python is not supported, the previous script only failed when pressing the formatting key combination. The new script will fail as soon as vim is opened and the configuration is parsed. I believe this is a regression for people who would like to use the same .vimrc file across different systems.

I think this is a valid concern that I didn't consider. Personally, I tend to prefer that vim tells me during startup if something is broken with Python, because I expect Python to work. But people might have different preferences.

v3 can be used in either way. The alternatives are described in the script. What I don't like about this solution is that the instructions got even longer. But I think this might be acceptable given the additional flexibility.

I think this generally seems like a good idea. However, I'd like to also keep the old version at least for a while for compatibility and because I think it is slightly easier to install until it can be used by Vundle etc.

v3 can be used in the old way. The only change is the rename (see below).

clang_format.py (5.27 KB)

So, remind me again, because it seems to have escaped from this discussion: Why is it a benefit to load this as a python module?

I think the capabilities to configure the plugin with global variables is obvious, but it seems unrelated to this being loaded as a module.

A few nits in your patch:

  • let g:clang_format_style = ‘{ BasedOnStyle: llvm, IndentWitdh: 4 }’

IndentWidth…

  • Use g:clang_format_binary or default ‘clang-format’.

  • binary = vim.eval(
  • ‘exists(“g:clang_format_binary”) ? g:clang_format_binary : “clang-format”’)

I think we should still default to a constant that is defined higher up in the file, possibly within the introductory paragraph. Yes, it can be configured setting the global options, but in some cases, it might be easier to change an easy to find constant in this file. Same for clang_format_style.

So, remind me again, because it seems to have escaped from this discussion: Why is it a benefit to load this as a python module?

My main reason was that with Python runtimepath handling (vim >= 7.3.1163), I think it's easier to install and use as a module.

I think the capabilities to configure the plugin with global variables is obvious, but it seems unrelated to this being loaded as a module.

True, and maybe it's not worth breaking compatibility.

+ # let g:clang_format_style = '{ BasedOnStyle: llvm, IndentWitdh: 4 }'
IndentWidth..

Thanks. Fixed.

+ # Use g:clang_format_binary or default 'clang-format'.
+ binary = vim.eval(
+ 'exists("g:clang_format_binary") ? g:clang_format_binary : "clang-format"')

I think we should still default to a constant that is defined higher up in the file, possibly within the introductory paragraph. Yes, it can be configured setting the global options, but in some cases, it might be easier to change an easy to find constant in this file. Same for clang_format_style.

How about the attached revision v4? It's backward compatible. The defaults for binary and style are specified after the imports. The binary can be specified in a global vim var. The style can be specified as an explicit function arg, a buffer-local vim var (useful for autocmd), or a global vim var. The install instructions are kept simple. Using a Python module is only mentioned later as an alternative.

Suggested commit message:

'''
clang-format (vim): Support configuration via vim vars and import as module

'clang-format.py' can now be configured via vim variables
'g:clang_format_binary' and 'b:clang_format_style' or
'g:clang_format_style'. Using vim variables is more flexible than
hard-coded values in the Python script. For example, the style can be
changed from vim on-the-fly without modifying the Python script.

Furthermore, 'clang-format.py' can now alternatively be imported and
used as a Python module. The change is backward compatible. When used
as a module, the style can explicitly specified when calling the
format() function.

Note also that unnamed buffers are now properly handled. Previously, a
Python exception was raised for unnamed buffers by subprocess.Popen,
because it was called with None for vim.current.buffer.name.

The implementation should work with vim 7.1 and later (maybe even 7.0).
It has been tested with 7.2.330 (Ubuntu 10.04), 7.3.429 (Ubuntu 12.04),
and MacVim 7.4.
'''

  Steffen

clang-format.py (4.76 KB)

> So, remind me again, because it seems to have escaped from this
discussion: Why is it a benefit to load this as a python module?

My main reason was that with Python runtimepath handling (vim >=
7.3.1163), I think it's easier to install and use as a module.

Maybe if someone is used to python modules. Effectively, this is just one
more line that needs to be put into the .vimrc (or alternatively, this file
needs to be put into a specific location that one might have to look up).
Installing by just binding this file to a key seems to be strictly one step
less.

I think the capabilities to configure the plugin with global variables is
obvious, but it seems unrelated to this being loaded as a module.

True, and maybe it's not worth breaking compatibility.

Yeah, we really need to be careful with this. I for one am pushing this
file to the workstations of many, many developers. Any change where they'd
need to change their .vimrc would be very tedious.

+ # let g:clang_format_style = '{ BasedOnStyle: llvm, IndentWitdh: 4
}'
> IndentWidth..

Thanks. Fixed.

> + # Use g:clang_format_binary or default 'clang-format'.
> + binary = vim.eval(
> + 'exists("g:clang_format_binary") ? g:clang_format_binary :
"clang-format"')
>
> I think we should still default to a constant that is defined higher up
in the file, possibly within the introductory paragraph. Yes, it can be
configured setting the global options, but in some cases, it might be
easier to change an easy to find constant in this file. Same for
clang_format_style.

How about the attached revision v4? It's backward compatible. The
defaults for binary and style are specified after the imports. The binary
can be specified in a global vim var. The style can be specified as an
explicit function arg, a buffer-local vim var (useful for autocmd), or a
global vim var. The install instructions are kept simple. Using a Python
module is only mentioned later as an alternative.

Looks good to me. If nobody else has objections, I will experiment a bit
with it (probably tomorrow) and the check it in.

Thanks for working on this!
Cheers,
Daniel

Suggested commit message:

This one works for me and also does not trigger require the user to perform a python include in the .vimrc file. So for me this patch works.

Thanks Steffen!

Tobias

> So, remind me again, because it seems to have escaped from this
discussion: Why is it a benefit to load this as a python module?

My main reason was that with Python runtimepath handling (vim >=
7.3.1163), I think it's easier to install and use as a module.

Maybe if someone is used to python modules. Effectively, this is just one
more line that needs to be put into the .vimrc (or alternatively, this file
needs to be put into a specific location that one might have to look up).
Installing by just binding this file to a key seems to be strictly one step
less.

> I think the capabilities to configure the plugin with global variables

is obvious, but it seems unrelated to this being loaded as a module.

True, and maybe it's not worth breaking compatibility.

Yeah, we really need to be careful with this. I for one am pushing this
file to the workstations of many, many developers. Any change where they'd
need to change their .vimrc would be very tedious.

+1 for not breaking compatibility.

-- Sean Silva