[cfe-dev] Slightly improved clang-format vim integration

[Daniel Jasper]

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.





On Tue, Sep 17, 2013 at 9:53 AM, Steffen Prohaska <prohaska at zib.de> wrote:

> 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.
> '''
>
>
> Tobias Grosser <tobias at grosser.es> wrote:
>
> >> 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.
>
>
> On Sep 16, 2013, at 8:12 PM, Daniel Jasper <djasper at google.com> wrote:
>
> > 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).
>
>
> Nico Weber <thakis at chromium.org> wrote:
>
> > FWIW you don't _have_ to rename the script, you could use `clang_format
> = __import__("clang-format")`
>
> True.  I briefly considered keeping the old name.  But it feels unnatural
> to import Python modules in that way.  Therefore, I propose to rename the
> script, even if means a minor breakage of compatibility (people have to
> change '-' to '_' in their mappings).
>
>         Steffen
>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130917/e12d3c21/attachment.html>