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

Fix process spawning error handling #3349

Merged
merged 2 commits into from
Aug 30, 2022
Merged

Fix process spawning error handling #3349

merged 2 commits into from
Aug 30, 2022

Conversation

PiergiorgioZagaria
Copy link
Contributor

As mentioned by @dead10ck in #2942 (comment) if something is printed to stderr it doesn't mean that the process failed, so we need to check the exit code and then print stderr if there is anything. This should also be valid when calling the shell from helix

dead10ck
dead10ck approved these changes Aug 7, 2022
View reviewed changes
Copy link
Member

@dead10ck dead10ck left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! It looks like a lot of this is duplicated, but I think it can be refactored later.

the-mikedavis
the-mikedavis reviewed Aug 8, 2022
View reviewed changes
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-view/src/document.rs Outdated Show resolved Hide resolved
helix-view/src/document.rs Outdated Show resolved Hide resolved
@PiergiorgioZagaria PiergiorgioZagaria mentioned this pull request Aug 9, 2022
Merged
@PiergiorgioZagaria
Copy link
Contributor Author

Now stderr is always logged, (if the status code is zero log::debug is used else if there was an error log::error is used). In case of errors stderr still returns the error, this is because for small error messages it's more convenient to read them in the editor without opening the logs

dead10ck
dead10ck approved these changes Aug 15, 2022
View reviewed changes
Copy link
Member

@dead10ck dead10ck left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks!

@archseer archseer added this to the 22.08 milestone Aug 18, 2022
@archseer archseer merged commit d2cec25 into helix-editor:master Aug 30, 2022
thomasskk pushed a commit to thomasskk/helix that referenced this pull request Sep 9, 2022
@PiergiorgioZagaria @thomasskk
Fix process spawning error handling (helix-editor#3349)
957776b 
* Fix process spawning error handling

* Log stderr in any case
jdrst pushed a commit to jdrst/helix that referenced this pull request Sep 13, 2022
@PiergiorgioZagaria @jdrst
Fix process spawning error handling (helix-editor#3349)
18efcb4 
* Fix process spawning error handling

* Log stderr in any case
herkhinah pushed a commit to herkhinah/helix that referenced this pull request Dec 11, 2022
@PiergiorgioZagaria
Fix process spawning error handling (helix-editor#3349)
2f750a2 
* Fix process spawning error handling

* Log stderr in any case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@the-mikedavis the-mikedavis the-mikedavis left review comments

@dead10ck dead10ck dead10ck approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
22.08
Development

Successfully merging this pull request may close these issues.
4 participants
@PiergiorgioZagaria @dead10ck @the-mikedavis @archseer