Make default editor message clear #207
Conversation
There was a problem hiding this comment.
Thank you for the PR. It's great to have your contribution.
A few minor nits and possible improvements:
- mention the installer in the commit subject line - e.g. Make installer default editor message clear
- mention in the message body: 'Fixes issue #1872'
- The new text forgets to mention that the default EDITOR is vim - the user can set it to be something else (which was the confusion in the original issue)
- can you check what the installer does if the user had previously set the EDITOR variable - does it leave it alone, or deliberately unset it?
- if the setting/unsetting of the EDITOR variable is too deep a rabbit hole, rephrase/trim the text to just the facts. maybe/e.g. "..Git fall back to the EDITOR environment variable. The default Editor is Vim."
If you force push the update then the PR will be refreshed.
No it doesn't changes the EDITOR variable. Just checked it. In this case, I think we should make two options for user, one for the default and one for vim because it could be misleading for the user otherwise. In the default option, we could mention that default editor in the Bash environment is Vim. |
Could we start with just this explanation, which would resolve the immediate issue. Maybe @zwilliams come confirm if that would be sufficient for the initial clarification. Then later a follow on PR could expand the choice, maybe it will need an option to specifically clear the EDITOR variable, and/or to set it to |
|
Okay so lets just write the facts in this PR. And then later we could discuss upon expanding. I will push the previously mentioned changes soon. |
aakriti-jain
force-pushed
the
installer-editor
branch
from
fc91b56 to
8e46ce4
Compare
Fixes git-for-windows/git#1872 Signed-off-by: aakriti-jain <aakritijain1102@gmail.com>
aakriti-jain
force-pushed
the
installer-editor
branch
from
8e46ce4 to
a8ddba1
Compare
|
The requested changes have been done @PhilipOakley . Heres a screenshot - Waiting for your review. |
|
Makes sense to me. The only improvement that I could recommend would be including a link to the git-var documentation, because it really spells out exactly what is going on with the different configuration options (there are five!) that can influence which editor Git uses. |
|
Excellent, thank you so much @aakriti-jain!
@zwilliams feel free to work on that. It should be as easy as adding a |
1 similar comment
This comment has been minimized.
This comment has been minimized.
The description of the editor option to choose Vim [has been clarified](#207) to state that this *unsets* `core.editor`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
I do understand the goal of the message on vim : this editor is definitely not for beginner, and this is a good idea to warn windows users, but the form is definitely clumsy if not insulting, and with a big value judgment. Please consider that vim is still a full featured, wide used editor (very popular among Golang developers for instance). Even some "modern" text editor or IDE like Sublime text or Intellij do offer a vim compatibility mode. Hard, indeed, unfamiliar, yes, but neither awkward nor obsolete. |
The installer doesn't state that it is obsolete. But awkward it is, the entire vim experience. I am a long-time vim user, and it is still my go-to editor on the console. And even for me, the handling is awkward at best. In any case, @jeromedoucet if you truly care, you will open a new Pull Request and not only add a comment to an already-closed Pull Request... |
This fixes the issue/feature request mentioned in git-for-windows/git#1872.
As suggested by @PhilipOakley , I have added an extra note to make it more clear what the Vim option actually does.
Signed-off-by: aakriti-jain aakritijain1102@gmail.com