Having an $EDITOR with arguments raises an exception

[davidparsson]

editor.edit('file') fails when the defined $EDITOR contains spaces:

$ touch /tmp/file
$ export EDITOR='vim -v'
$ python -c 'import editor; editor.edit("/tmp/file")' 
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "editor.py", line 103, in edit
    proc = subprocess.Popen(args, close_fds=True, stdout=stdout)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 710, in __init__
    errread, errwrite)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1335, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory

After removing the space, it works:

$ export EDITOR='vim'
$ python -c 'import editor; editor.edit("/tmp/file")'

Originally (and improperly) reported here as an Alembic issue.

[eugene-eeo]

@davidparsson I forgot about the old behaviour, but this is most likely ibecause to avoid arbitrary code injection, the library uses subprocess.Popen and passes a list according to the following spec:

program, *args = [a1,a2,...]

the OS then resolves the program executable, e.g. looking in $PATH, etc. and tries to execute the program with the name program along with args as arguments (the strings you find in sys.argv). In this case, vim -v is not a valid program as it tries to execute vim<SPACE>-v, which is not found on your system.

If you're using the latest codebase, as in the master branch, it should not be an issue.

EDIT: I was mistaken as I briefly skimmed through the code. Sorry!

[davidparsson]

@eugene-eeo, are you saying that this functionality is intended and not likely to change? I'm seeing this on the master branch as well.

[eugene-eeo]

@davidparsson I can't read minds, but probably yes. Relevant code: https://github.com/fmoo/python-editor/blob/master/editor.py#L83

[davidparsson]

All right, if so then that's a shame. Since having a space in an editor is supported by most Unix commands, I think it would make sense to support that here as well.

[eugene-eeo]

I think it can be implemented quite easily using the shlex module. Give me a moment and I'll hack up a version and see if it suits your needs.

@davidparsson does this look good? https://gist.github.com/eugene-eeo/f0baa1e79b5efbe240c2d5b3d9a899e8

[fmoo]

I think I'm a fan of making this use case work. Does this break editors that have a whitespace in their executable name?

Not sure that's a use case we need to care about, but I guess we can add some heuristics if we need to support it.

My main concern with the PR that I initially looked at was that it changed a couple of other things that seemed unrelated, but I haven't looked too closely yet.

On Nov 11, 2016, at 7:27 AM, Eeo Jun notifications@github.com wrote:

I think it can be implemented quite easily using the shlex module. Give me a moment and I'll hack up a version and see if it suits your needs.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

[eugene-eeo]

shlex handles editor\ binaries\ with\ spaces perfectly.

>>> import shlex
>>> shlex.split('abc\ def')
['abc def']

[davidparsson]

@eugene-eeo, nearly, but not quite. The args passed to Popen for that code are in my case ['subl --wait', '--wait', 'file.txt'] which raises the same exception.

[eugene-eeo]

@davidparsson it should work now :)

[davidparsson]

@eugene-eeo, indeed it does. Thanks!

Do you think that this will be a part of a future release?

[eugene-eeo]

@davidparsson that will be up to @fmoo. However I don't see any drawbacks from this and it makes the library more Unix-friendly so my best guess would be that it would be part of a future release.

[fmoo]

I published a 1.0.2, but setuptools is throwing a hissy fit and not letting me upload via cli.

So I tried uploading via the UI, but pip doesn't seem to see it. Any ideas?

On Nov 16, 2016, at 3:03 PM, Eeo Jun notifications@github.com wrote:

@davidparsson that will be up to @fmoo. However I don't see any drawbacks from this and it makes the library more Unix-friendly so my best guess would be that it would be part of a future release.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[eugene-eeo]

Can you share the warning that setuptools displayed? Maybe you've previously published the same version. BTW, my PR that you merged does not include this fix. Let me know if I should include the support for spaces as well, and I'll drop it in and write some tests.

[fmoo]

It was an authentication error. Which is weird because I can use the credentials to login through the web UI.

On Nov 17, 2016, at 4:42 AM, Eeo Jun notifications@github.com wrote:

Can you share the warning that setuptools displayed? Maybe you've previously published the same version.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.