the setup instructions seem to trail off

autopep8 1.0 and later support --range line line, which will give much better results than this hack. Unfortunately, Ubuntu seems to be stuck on a version almost 2 years old (0.9.1) which doesn't support it, so I could see people being stuck on an old version without it. I just filed https://bugs.launchpad.net/ubuntu/+source/autopep8/+bug/1414258 asking Ubuntu to update.

Could you file a feature request along with these changes to support true range formatting, with this as a fallback? "autopep8 --version" runs pretty quick for me, so I don't think running that once per vim session to detect native range support would be a problem.

how do u think it will be better to make that choice -- setting plugin.Flag manually or reading autopep --version output once and caching it?

If reading autopep8 --version works well, I'd go with that approach. Seems like it will.

btw, while i gonna store cached value in Flag, then it will be possible to set it manually

or Flags are not suitable for such purpose and intended to be only for storing some default settings, but not any variable data?

Using a flag like that wouldn't be unreasonable, except I figured it should be lazy and flag initialization should be eager (once at plugin installation time). Even though it's a quick check, doing it for N formatters plus the executable() checks all at once might not be.

i mean to add that version check not on plugin initialization but inside FormatRange function, like:

if !s:plugin.Flag('autopep8_version')
 s:plugin.Flag('autopep8_version', GetVersion())

Yeah, I thought lazy flag initialization like that might get confusing, but maybe not. Anyway, my vote would be to wait to add a flag until someone reports they need to override the behavior.

am i understood right, you mean to initialize version flag like that but not to set it manually by default?

What I was suggesting is instead of using a flag, for now use a script-local variable like

let l:executable = s:plugin.Flag('autopep8_executable')
if !exists('s:autopep8_supports_range')
 let l:version_output =
 \ maktaba#syscall#Create([l:executable, '--version']).Call().stderr
 let s:autopep8_supports_range =
 \ matchlist(l:version_output, '\m\Cautopep8 \(\d\+\)\.')[1] >= 1
endif

Then, if for some reason a user reports the version check is not right for them and they'd rather configure it manually, it's easy to replace the script-local variable with flag operations.

got it, thanks

oops, just noticed what autopep8 don't output errors like that.

actually i am not even sure if it can return unsuccessful result when using stdin at all (when it faces wrong syntax or smth it is just ignoring it)

Yeah, it's fine to get rid of the try/catch and just let the ShellError bubble up. It's caught and handled in codefmt. You can add a @throws ShellError line into the vimdoc for good measure.

Can you add a comment that the '-' argument means stdin and put the '-' at the end of the list of args (I got a bit confused by the '-', '--range' list)

I didn't find it too confusing, but it could be more obvious at a glance if the lines were broken up differently:

 let l:cmd = [
 \ l:executable,
 \ '--range', ''.a:startline, ''.a:endline,
 \ '-' ]

Matt, you still think a comment would be necessary like that? Was it more that you didn't notice the '-' or just didn't know what to make of it?

Didn't know what to make of it. I thought it was similar to the bash things
which say "all arguments after this one are files" which confused me and
sent me off to read the docs.

Matt
On Jan 26, 2015 11:51 AM, "David Barnett" notifications@github.com wrote:

In autoload/codefmt.vim
#4 (comment):

• " @throws ShellError
• function s:autopep8.FormatRange(startline, endline) abort
• " Hack range formatting by formatting range individually, ignoring context.
• let l:executable = s:plugin.Flag('autopep8_executable')
• if !exists('s:autopep8_supports_range')

•  let l:version_output =
  

•  \ maktaba#syscall#Create([l:executable, '--version']).Call().stderr
  

•  let s:autopep8_supports_range =
  

•  \ matchlist(l:version_output, '\m\Cautopep8 (\d+).')[1] >= 1
  

• endif

• call maktaba#ensure#IsNumber(a:startline)
• call maktaba#ensure#IsNumber(a:endline)
• let l:lines = getline(1, line('$'))

• if s:autopep8_supports_range

I didn't find it too confusing, but it could be more obvious at a glance
if the lines were broken up differently:

 let l:cmd = [
 \ l:executable,
 \ '--range', ''.a:startline, ''.a:endline,
 \ '-' ]

Matt, you still think a comment would be necessary like that? Was it more
that you didn't notice the '-' or just didn't know what to make of it?

—
Reply to this email directly or view it on GitHub
https://github.com/google/vim-codefmt/pull/4/files#r23544410.

Can you add a comment on what cases you're handling here? To my eye the logic looks a little weird without any info about expected output.

BTW, it reads a little weird not having a space after the \.

so as i described in commit message, in some configurations version is being output into stdout, in some others -- to stderr.
i have no clue why that happens (it seems what i have the same library versions on both PCs there i tested it, but maybe some different environment variables or smth)

yup, that's a typo, i will fix it