<div dir="ltr">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?<div><br></div><div>I think the capabilities to configure the plugin with global variables is obvious, but it seems unrelated to this being loaded as a module.</div>
<div><br></div><div>A few nits in your patch:</div><div><br></div><div>+ # let g:clang_format_style = '{ BasedOnStyle: llvm, IndentWitdh: 4 }'<br></div><div>IndentWidth..<br></div><div><br></div><div><div>+ # Use g:clang_format_binary or default 'clang-format'.</div>
<div>+ binary = vim.eval(</div><div>+ 'exists("g:clang_format_binary") ? g:clang_format_binary : "clang-format"')</div></div><div><br></div><div>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.</div>
<div><br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Sep 17, 2013 at 9:53 AM, Steffen Prohaska <span dir="ltr"><<a href="mailto:prohaska@zib.de" target="_blank">prohaska@zib.de</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for the comments.<br>
<br>
I've attached v3 of the script that hopefully addresses all your comments (see inline replies below). Suggested commit message:<br>
<br>
'''<br>
clang-format (vim): Support config via 'g:' globals, importing as Python module<br>
<br>
'clang-format.py' is changed such that it can either be executed<br>
directly as before or imported and used as a Python module. Both<br>
alternatives are described in the introductory comment in the script.<br>
Since the natural way of importing Python modules ('import <module>')<br>
only works if the module name does not contain dashes, the file is<br>
renamed to 'clang_format.py'.<br>
<div class="im"><br>
Configuration now works via global vim variables 'g:clang_format_binary'<br>
</div>and 'g:clang_format_style'. Using global vim variables is more flexible<br>
<div class="im">than hard-coded values in the Python script. For example, the style can<br>
</div>be changed from vim on-the-fly without modifying the Python script.<br>
<div class="im"><br>
The implementation should work with vim 7.1 and later (maybe even 7.0).<br>
It has been tested with 7.2.330 (Ubuntu 10.04), 7.3.429 (Ubuntu 12.04),<br>
and MacVim 7.4.<br>
<br>
Note also that unnamed buffers are now properly handled. Previously,<br>
a Python exception was raised for unnamed buffers by subprocess.Popen,<br>
because it was called with None for <a href="http://vim.current.buffer.name" target="_blank">vim.current.buffer.name</a>.<br>
'''<br>
<br>
<br>
</div><div class="im">Tobias Grosser <<a href="mailto:tobias@grosser.es">tobias@grosser.es</a>> wrote:<br>
<br>
>> Python has also been required by the previous version of the script. So my proposed changes don't make the situation worse.<br>
><br>
</div><div class="im">> 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.<br>
<br>
</div>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.<br>
<br>
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.<br>
<div class="im"><br>
<br>
On Sep 16, 2013, at 8:12 PM, Daniel Jasper <<a href="mailto:djasper@google.com">djasper@google.com</a>> wrote:<br>
<br>
> 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.<br>
<br>
</div>v3 can be used in the old way. The only change is the rename (see below).<br>
<br>
<br>
Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>
<br>
> FWIW you don't _have_ to rename the script, you could use `clang_format = __import__("clang-format")`<br>
<br>
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).<br>
<span class="HOEnZb"><font color="#888888"><br>
Steffen<br>
<br>
</font></span><br><br>
<br>
<br></blockquote></div><br></div>