python-editor icon indicating copy to clipboard operation
python-editor copied to clipboard

Support editors with arguments, using shlex

Open davidparsson opened this issue 8 years ago • 14 comments

Fixes #11.

davidparsson avatar Oct 16 '17 07:10 davidparsson

As-is, this breaks editors with spaces in their names or paths. What if we wrap the Popen call with try/catch (re-raising if it's not ENOENT or the editor has no spaces), and try other variations inside the catch?

fmoo avatar Oct 16 '17 16:10 fmoo

@fmoo, is editors with (unescaped) spaces a thing that need to be supported? I don't think it should be, at least not on *nix.

Even Bash's CTRL+x, CTRL+e does not support EDITOR='some editor', but supports EDITOR='some\ editor'. The same goes for git. Or are there other cases where it makes sense to support this?

davidparsson avatar Oct 17 '17 07:10 davidparsson

What about:

def edit(filename=None, args=(), contents=None, use_tty=None):
    editor = get_editor()
    args = [editor] + (args or get_editor_args(...))
    ...

We assume that if you're gonna pass in additional arguments args then you know what you're doing. This solves issues such as "Where do the additional arguments go".

eugene-eeo avatar Oct 18 '17 14:10 eugene-eeo

Okay, let's make sure we don't misunderstand each other, because I see that #11 may be misinterpreted. When I opened #11 I did not mean to add support for binaries with spaces in their names, but to allow passing arguments in the $EDITOR variable. This conforms with how the $EDITOR variable is handled by other tools I've used, and has the (intentional) side-effect of breaking editors with (unescaped) spaces in their names.

But if you're still convinced that support for both cases are needed, despite it not being the "standard way", please let me know.

davidparsson avatar Oct 19 '17 08:10 davidparsson

Ok that makes a ton of sense now. :+1: My bad for rushing into it; can you verify the behaviour of most tools when we pass in arguments to the $EDITOR variable? I.e. are there any additional arguments passed? Thanks.

eugene-eeo avatar Oct 19 '17 11:10 eugene-eeo

Apart from bash and git (above), a few Googles found this:

Bash, git and emacs are all pretty prominent software projects, but apart from that, if you're not convinced, I'm not really sure what more tools to test. What else uses $EDITOR?

davidparsson avatar Oct 19 '17 14:10 davidparsson

Yeah, I came across the Emacs one which imho is the most compelling argument for supporting this.

The one thing I want to consider specially (and not break) is the "default args" behavior for various text editors (this is similar to what hg has implemented). It's not clear to me whether args in $EDITOR should override or augment the default args we provide though I am thinking augment. I'm curious about others' thoughts on this.

If the current behavior of the two patches you sent David seem reasonable with this, I'd like to take both revs and do a major version bump to 2.0 since they're technically "breaking" API changes.

On Thu, Oct 19, 2017 at 7:20 AM, David Pärsson [email protected] wrote:

Apart from bash and git (above), a few Googles found this:

Bash, git and emacs are all pretty prominent software projects, but apart from that, if you're not convinced, I'm not really sure what more tools to test. What else uses $EDITOR?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fmoo/python-editor/pull/15#issuecomment-337923253, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIwkGyP480wXRCaRwiR73XBqpRbRl1Fks5st1rHgaJpZM4P6OHM .

-- Peter Ruibal

fmoo avatar Oct 19 '17 16:10 fmoo

I'm not sure what all current default arguments for all editors do, but I'm a big fan of sane defaults, so I think the augment approach seems like a reasonable way to go. Do you know how hg handles this case? Is it likely that any user would want to remove those default arguments?

I fully agree about the major version bump as well

davidparsson avatar Oct 20 '17 07:10 davidparsson

Hi any updates on this issue? Would like to be able to open vscode which requires -w -n as args. Currently just hacked this by adding code with those args to the get_editor_args function...

    elif editor == "code":
        return ["-w", "-n"]

Don't really want to create a fork just to support this...

josh-gree avatar Apr 23 '19 09:04 josh-gree

@josh-gree, my workaround for this was to create a scripts that opens the editor with arguments.

My /usr/bin/editor could contain something like:

#!/bin/sh
code -w -n $1

and then I've set $EDITOR to /usr/bin/editor.

Disclaimer: I've not tested this example, but you probably get it.

davidparsson avatar Apr 23 '19 10:04 davidparsson

@josh-gree if you put up a PR for get_editor_args defaults update for vscode, I'll take it.

fmoo avatar Apr 23 '19 16:04 fmoo

(I'd do a quick release as 1.0.4 and then look at pulling this change in as 2.0)

fmoo avatar Apr 23 '19 16:04 fmoo

@fmoo PR added #26

josh-gree avatar May 03 '19 15:05 josh-gree

Any news on the 2.0 release?

23Skidoo avatar Feb 24 '22 13:02 23Skidoo