lucc / khard

Console vcard client
https://khard.readthedocs.io/en/latest/
GNU General Public License v3.0
600 stars 65 forks source link

Properly split $EDITOR env variable #314

Closed m-zat closed 2 years ago

m-zat commented 2 years ago

This commit introduces a change which allows to treat properly the $EDITOR variable. Before this was taken as a string and passed to subprocess.Popen. This worked when $EDITOR was just a command with no arguments appended. When arguments are appended subprocess.Popen requires them to be passed as list. This commit introduces the logic for splitting the $EDITOR string as a list recommended by subprocess.Popen documentation.

This commit solves #293

lucc commented 2 years ago

Can you rebase on the latest development branch so that ci runs also for this pull request (sorry I missed to specify this in the workflow file).

Also can you change the wording in your commit: You can simple use the imperative tone without the this commit does that ... phrase (like you already do in your summary). Maybe you can just drop the first sentence. Also $EDITOR is not consistently in code tags.

lucc commented 2 years ago

@m-zat how did you merge this? Did you have some button in the github interface available? I can not see that you have any access rights on the repository. Or did github somehow merge it automatically after the CI completed?

lucc commented 2 years ago

Oh now I see you did not put any commits in you develop branch after the rebase so the PR was empty.

Can you reopen the PR with the commit ed42d04c97230898a21187dfc9ac7e135fb24981 or the proper rebase of it? In case you don't have a local branch with that commit any longer you can just create one with git branch fix-editor ed42d04c97230898a21187dfc9ac7e135fb24981 as your local repo probably still has the old commit (unless you ran git gc).

lucc commented 2 years ago

@m-zat never mind I found a way to download the commit from github: https://github.com/lucc/khard/commit/ed42d04c97230898a21187dfc9ac7e135fb24981.patch

I amended and merged the commit myself, so this is done.

m-zat commented 2 years ago

Thanks!