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

on non-zero exit display stderr in message area #34

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

Profpatsch
Copy link
Contributor

This is a proposal to fix #32

Instead of throwing an error (which disables direnv-mode for all
buffers), display the stderr of direnv in the status line.

If an envrc was not yet allowed, this will provide a better user
experience, prompting the user with a message that is easy to ignore.

cc @jvshahid

Copy link
Owner

@wbolster wbolster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for looking into this.

what about popping up the *direnv* (with space in front) buffer instead? it would show everything.

also it seems the temporary file is not deleted in all cases.

not using a temp file but showing the buffer instead would avoid all that i think?

@Profpatsch
Copy link
Contributor Author

what about popping up the *direnv* (with space in front) buffer instead? it would show everything.

yeah, but we only really want stderr, not the potentially very large json output plus an error tucked to the end. Popping up a buffer is very distracting, that’s what the message area is for normally. You can always check *Messages* if you accidentally discard the message.

also it seems the temporary file is not deleted in all cases.

good catch, will move that to the very end of that block.

@Profpatsch
Copy link
Contributor Author

This is ready from my side.

@wbolster
Copy link
Owner

wbolster commented Mar 6, 2019

yeah, but we only really want stderr, not the potentially very large json output plus an error tucked to the end.

is that really possible in practice? having an error and json output?

@Profpatsch
Copy link
Contributor Author

Well yeah, it happens when you run direnv export json and the .envrc is not allowed, see direnv/direnv#467 for a description.

@wbolster
Copy link
Owner

wbolster commented Mar 8, 2019

ok, fair enough.

this pr does a few things:

  1. makes direnv errors not fatal, which means the post-command hook survives

  2. it moves stderr output away from the hidden *direnv* buffer.

  3. it changes the message shown in the minibuffer.

doing 1 is fine, it doesn't ‘cripple’ emacs as a whole just by opening a file in directory tree containing a not-yet-allowed .envrc file, and that is an improvement.

doing 2 seems not needed, and seems like a regression. the *direnv* buffer is a nice go-to place for troubleshooting. mixed output + parsing json works fine with the current code, since (json-read-object) starts reading at point. having to read both *Messages* and *direnv* buffers is not user-friendly.

doing 3 is fine if there's only info added. but the main ‘there is a problem’ message has disappeared.

what do you think?

sorry for being nitpicky; i'd rather keep things as simple and explicit as possible. this is one of many projects for me, and this helps maintenance burden (code + support).

so let me say it out loud: thanks for improving emacs-direnv, i appreciate it a lot. 👍

@Profpatsch
Copy link
Contributor Author

2. it moves stderr output away from the hidden *direnv* buffer.

I agree, we should also add stderr to the direnv buffer.

the *direnv* buffer is a nice go-to place for troubleshooting.

The problem here was that it opened the direnv buffer, meaning the user has to make an effort to close that buffer again each time they switch to a buffer that has not yet run direnv allow. Modal dialogues of old come to mind, “an error has happened <ok>”.

Pasting stderr to the message region means two things:

  • the user can hide the message by hitting any key, no further action needed
  • the user gets the gist of the problem (stderr) with one glance, no need to open another buffer
  • it uses the default mechanisms for warnings & errors, so it integrates in the default Emacs workflow

but the main ‘there is a problem’ message has disappeared.

I write “direnv exited ”, but that can be made more problem-y if you want.

Instead of throwing an error (which disables direnv-mode for all
buffers), display the stderr of direnv in the status line.

If an envrc was not yet allowed, this will provide a better user
experience, prompting the user with a message that is easy to ignore.
@Profpatsch
Copy link
Contributor Author

Profpatsch commented Mar 8, 2019

I appended the stderr to " *direnv*" and added a line to the message.

To be honest it took me a while to understand how to open " *direnv*", because buffers with spaces are not listed or autocompleted anywhere, you have to type their name blindly.

@wbolster
Copy link
Owner

wbolster commented Mar 8, 2019

To be honest it took me a while to understand how to open " *direnv*", because buffers with spaces are not listed or autocompleted anywhere, you have to type their name blindly.

ivy (and possibly other completion frameworks) will list them after typing the initial space.

i used a hidden buffer because it seems the right approach for ‘unintrusive’ buffers, but in practice it's just causing confusion, since people actually do care about that buffer quite often.

i'd happily accept a pull request that removes the space and updates the readme 😉

Copy link
Owner

@wbolster wbolster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, looks good!

@wbolster
Copy link
Owner

error handling has further improved. see #41 and #42.

errors (e.g. unapproved .envrc files) will trigger emacs warnings, but direnv-mode will keep working. and with the new direnv-allow command (see #43) this will be easy to fix. :)

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.

The post-command-hook will fail if the .envrc file isn't allowed
2 participants