Use git config editor instead of ENVIRONMENT variable

[rorybyrne]

Hey, thanks for the PR! I tried this out, and got an error:

I think you should catch the GitException in PlanService, and in the except block fall back to the default.

Also, for the fallback can you use os.environ.get("EDITOR", "vim") instead of just vim?

[Goorzhel]

git config returns 1 if the key isn't found, and I suspect @synek, like myself, doesn't have a core.editor set.

My way was to use _run_command's Optional[str]:

        try:
            value = self._run_command(cmd)
        except subprocess.CalledProcessError as e:
            if e.returncode == 1:
                value = None
            else:
                raise GitException(f'Failed to get config: "{config}"') from e

        return value

but that makes mypy sad:

        editor = self._git_service.get_config('core.editor')  # if I str(...) here, then `editor == 'None'` is possible
        if not editor:
            editor = os.environ.get('EDITOR', 'vim')
        if not is_installed(editor):  # mypy: Argument <...> has incompatible type "Optional[str]"; expected "str"
            raise RuntimeError("Couldn't find an editor installed on your system.")

EDIT 07:04Z: Ninjaed again. FWIW, Akash's solution is way simpler, so carry on.

[akashRindhe]

I think you should catch the GitException in PlanService, and in the except block fall back to the default.

Missed out on doing negative testing locally for this.

Also, for the fallback can you use os.environ.get("EDITOR", "vim") instead of just vim?

Done.

[rorybyrne]

@akashRindhe Thanks for updating that. Could you add a test or two in tests/service/test_git_service.py? Coverage is a little low so I'm trying to add a couple of tests in each PR.

Also, can you add a -> Optional[str] type hint in your get_configured_editor() method?

@Goorzhel Bad luck! :laughing: I like your implementation though. If there's another issue you want to tackle then we can get it it merged pretty quickly.

As a side note, this is why we should ask/announce in the issue before opening a PR.

[rorybyrne]

Thanks @akashRindhe, I'll merge this now if you think it's done?

[akashRindhe]

Thanks @akashRindhe, I'll merge this now if you think it's done?

Yep, looks fine now. Thanks!