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.

Sign up for GitHub

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

Jump to bottom

Always show both stdout and stderr for code blocks #189

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Always show both stdout and stderr for code blocks #189

wants to merge 4 commits into from

Conversation

mmlb
Copy link

@mmlb mmlb commented Sep 17, 2022

Changes Introduced

  • Always display stdout from code blocks (even if an error occurs) as it may be relevant
  • Display stderr from code blocks as it may be relevant too

@mmlb
Fix imports in model.go
2a0aea8 
I noticed that the non-stdlib imports were split into 2 groups, seemingly
randomly. No other file does this so I just brought it back to one group
and let goimports sort it.
@mmlb
code: Always display the output of code blocks
9035d62 
Any output that is printed may still be useful even if the command returns
an error so lets display it. Printing out even in case of error is fine,
if there's no output and output.Write handles that fine. I tested this by
trying to run a code block for a language I do not have the compiler for.
@mmlb
code: Show both stdout and stderr
8ae6fd2 
Code blocks may try to print to stderr which is currently dropped.
@mmlb mmlb force-pushed the show-stderr-and-do-not-drop-stdout branch from 51b5f71 to 8ae6fd2 Compare September 17, 2022 01:48
@mmlb
Copy link
Author

mmlb commented Sep 19, 2022

Gentle ping to @maaslalani

@maaslalani
Copy link
Owner

Thanks @mmlb!

@maaslalani
Copy link
Owner

maaslalani commented Sep 19, 2022 •
edited
Loading

@mmlb I'm not a huge fan of the error messages produce by the stderr because of the tmp file. I wonder if there's a better solution (perhaps a different keybinding, to also receive the stderr)?

image

Alternatively, I wonder if we can scrub the file name from the Output which would solve the issue.

@mmlb
Copy link
Author

mmlb commented Sep 19, 2022

Yeah agree about that. I would not like to have a different key binding, I can take a look at dropping the filename.

@mmlb
Copy link
Author

mmlb commented Sep 25, 2022

@maaslalani I've pushed a quick hack to get rid of the name (only tested on go and bash atm) how's this error output look?

image

I'm not a fan of the # command-line-arguments and some of the other stuff but I don't think it should be removed, if the $tool adds it then it should be there I think.

@tploss
Copy link

tploss commented Feb 6, 2023

Gentle ping, @maaslalani and @mmlb
Any update on this PR?

@mmlb
Copy link
Author

mmlb commented Feb 7, 2023

Looking for any feedback

@maaslalani
Copy link
Owner

Hey @mmlb, I don't like the idea of the OutputReplacement since it makes adding a language have more friction. I'm not sure the correct approach but I think my thinking would be a separate key bind / extra step to reveal the full error so that it doesn't cloud up the presentation by accident during a live presentation.

@maaslalani
Copy link
Owner

Another option is to log to a temporary file which people can inspect?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mmlb @maaslalani @tploss