Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Give better error message for editor enviroment variable if variable is empty #426

Closed
wants to merge 1 commit into from

Conversation

WizardOhio24
Copy link
Contributor

This is link to a discussion in #419.

This gives a better error message for opening an external editor. This is also linked to #400, but is different because this gives a specific error message, saying the one of the environment variables must be empty.

@WizardOhio24 WizardOhio24 force-pushed the editor-blank branch 2 times, most recently from b52aa5f to e74ad09 Compare November 14, 2020 21:39
@pm100
Copy link
Contributor

pm100 commented Nov 14, 2020

here is what git says

image

@WizardOhio24
Copy link
Contributor Author

I do not get that. Look at the lines changed, this PR could not cause that, it only changes a string which would be output in the event of an error, where the error can only be if the environment variables are empty:
EditorError

To see this, use the command GIT_EDITOR="" cargo run and try to open the editor(for example, by committing then ^e). What did you do to produce that error? It is definitely unrelated to this considering what this changes, so there should be another PR for it.

@WizardOhio24
Copy link
Contributor Author

WizardOhio24 commented Nov 15, 2020

Wait, ignore my last comment, just realised you meant git cli.

That's fine for git to say that, but it is a cli and GitUI is a UI and so should try to be eaiser to use than git, that's the whole point. And the error that git gives is not helpful if an environment variable is not set correctly. This PR gives the user a better error message so they should be able to resolve the issue without ever having to go to the internet. I imagine that if a git cli user saw that git cli error and was relatively unfamiliar with git, they would spend a bit of time searching until they found the answer.

@pm100 pm100 mentioned this pull request Dec 7, 2020
@extrawurst
Copy link
Owner

@WizardOhio24 sorry that my review delay seem to offend you. closing without any comments seems not constructive. lets keep this open until its reviewed

@extrawurst
Copy link
Owner

closed in favour of 156776a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants