dandavison / open-in-editor

Open a local file from a URL at a line number in an editor/IDE
55 stars 8 forks source link

feat: add Helix text editor support #17

Closed eugenesvk closed 1 year ago

eugenesvk commented 1 year ago

By the way, is the relaxed matching of executables ("code" in executable_path instead of executable_path.endswith("code")) intentional? I've encountered this issue when I've added this editor at the end, but then the o condition (is this some catch-all editor?) interfered since path had o in it

dandavison commented 1 year ago

It is deliberately relaxed, because the executable path might be something like /path/to/my-editor --some-arg, so e.g. we can't assume endswith. I'm open to tightening this up. E.g. demand a whitespace character after the editor name and/or a path separator or nothing before?

o is a contributor's editor project: https://github.com/dandavison/open-in-editor/pull/5 We should tighten that one up or remove it if it's causing problems.

dandavison commented 1 year ago

Thanks!

eugenesvk commented 1 year ago

It is deliberately relaxed, because the executable path might be something like /path/to/my-editor --some-arg

but this is already dealt with in the preceding .index(" -") check arguments_start = executable_path_with_arguments_maybe.index(" -") and then executable_path = executable_path_with_arguments_maybe[:arguments_start].rstrip()

so the endswith would only operate on the no-arg rstripped path?

dandavison commented 1 year ago

But someone's editor path might use an argument without a - prefix? The aim was to keep this code very simple and pragmatic since what it's doing (inferring editors by sniffing env vars) is inherently questionable, and since there are no tests. If we're tightening things up I'd like that to be accompanied by tests; possibly only worth it if it's fixing a known problem?

eugenesvk commented 1 year ago

Yeah, the no--prefix is an issue, I guess then adding space as a requirement is a good alternative, would be more rare to have o in a path (but would be unfair to remove an editor just because of this)