text editor selection: follow unix defacto standards #356

plgruener · 2021-05-05T02:03:32Z

plgruener commented

2021-05-05 02:03:32 +00:00

Currently, tea only supports the $EDITOR env var to open the user's preferred editor (used for reviewing pull requests).

Standard *nix practice is, however, to check for $VISUAL first and only then use $EDITOR as fallback.
This is also done by Git itself, see man git-var(1).
(Actually, the order there is $GIT_EDITOR > core.editor > $VISUAL > $EDITOR > vi)

Currently, `tea` only supports the $EDITOR env var to open the user's preferred editor (used for reviewing pull requests). Standard \*nix practice is, however, to check for $VISUAL first and only then use $EDITOR as fallback. This is also done by Git itself, see man git-var(1). (Actually, the order there is $GIT_EDITOR > core.editor > $VISUAL > $EDITOR > vi)

plgruener added 1 commit 2021-05-05 02:03:33 +00:00

continuous-integration/drone/pr Build is passing

Details

a1b34187f7 support $VISUAL, prefer it over $EDITOR

Currently, `tea` only supports the $EDITOR env var to open the user's
preferred editor (used for reviewing pull requests).

Standard *nix practice is, however, to check for $VISUAL first and only
use $EDITOR as fallback.
This is also done by Git itself, see man git-var(1).
(Actually, the order is $GIT_EDITOR > core.editor > $VISUAL > $EDITOR > vi)

lunny approved these changes 2021-05-05 10:03:05 +00:00

noerw commented

2021-05-05 10:15:13 +00:00

Maybe we also should change the fallback from vim to vi? (The latter is installed by default on many systems, unlike vim.

Maybe we also should change the fallback from `vim` to `vi`? (The latter is installed by default on many systems, unlike vim.

plgruener commented

2021-05-05 11:10:43 +00:00

Maybe we also should change the fallback from vim to vi? (The latter is installed by default on many systems, unlike vim.

Yes, that would probably be the safer choice.

Should I open a new PR or edit this one?

> Maybe we also should change the fallback from `vim` to `vi`? (The latter is installed by default on many systems, unlike vim. Yes, that would probably be the safer choice. Should I open a new PR or edit this one?

noerw commented

2021-05-05 11:43:07 +00:00

@plgruener Just add it here :) Thanks!

noerw changed title from ~~support $VISUAL, prefer it over $EDITOR~~ to text editor selection: follow unix defacto standards

2021-05-05 11:44:41 +00:00

noerw added this to the v0.8.0 milestone 2021-05-05 11:44:46 +00:00

noerw added the

kind

enhancement

label 2021-05-05 11:44:52 +00:00

plgruener added 1 commit 2021-05-05 13:07:43 +00:00

38b6c826a0 change default text editor to vi

noerw approved these changes 2021-05-05 13:56:11 +00:00

plgruener requested review from lunny 2021-05-05 14:06:13 +00:00

6543 approved these changes 2021-05-05 23:37:43 +00:00

6543 commented

2021-05-27 16:29:13 +00:00

@plgruener can you "Update" the branch?

plgruener commented

2021-05-27 17:08:08 +00:00

Sorry, I didn't know I had to do anything else other than waiting (for the CI checks and then some of you merging the PR).

@plgruener can you "Update" the branch?

You mean rebasing it on top of master? I sure can, but then you'd have to review it again, wouldn't you?
Or do you mean the pending review? I don't know how I can un-request that.

Sorry, I didn't know I had to do anything else other than waiting (for the CI checks and then some of you merging the PR). > @plgruener can you "Update" the branch? You mean rebasing it on top of master? I sure can, but then you'd have to review it again, wouldn't you? Or do you mean the pending review? I don't know how I can un-request that.

6543 commented

2021-05-27 22:28:21 +00:00

No problem just merge current master into this feature branch, there should be a button ? somewhere called "update branch" witch would do the same

plgruener force-pushed visual_patch from 38b6c826a0 to e709df6259

2021-05-28 00:43:59 +00:00

Compare

plgruener commented

2021-05-28 01:14:22 +00:00

I did, but your CI is still failing.

You mean rebasing it on top of master? I sure can, but then you'd have to review it again, wouldn't you?

To clarify, this was not a technical question, but one about workflow and principle. What I meant:
as soon as I modify the PR – either by introducing a new commit, back-merging (with potential merge-conflicts) or rebasing – I instantly invalidate the (already approved) reviews, because I could have introduced another, potentially breaking or malicious, change.
So I was just wondering how you handled that.

I did, but your CI is still failing. > You mean rebasing it on top of master? I sure can, but then you'd have to review it again, wouldn't you? To clarify, this was not a technical question, but one about workflow and principle. What I meant: as soon as I modify the PR – either by introducing a new commit, back-merging (with potential merge-conflicts) or rebasing – I instantly invalidate the (already approved) reviews, because I could have introduced another, potentially breaking or malicious, change. So I was just wondering how you handled that.

6543 commented

2021-05-28 02:13:04 +00:00

Lgtm would still count, since the person who wil merge will have a final look anyway

6543 merged commit 3129e60a73 into master

2021-06-21 12:08:27 +00:00

6543 referenced this issue from a commit

2021-06-21 12:08:28 +00:00

text editor selection: follow unix defacto standards (#356)

Sign in to join this conversation.

No reviewers

No Milestone

No Assignees

4 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: gitea/tea#356