-
Notifications
You must be signed in to change notification settings - Fork 94
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
Allow using stderr for prompt output #109
Closed
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Laravel sends an instance of
\Illuminate\Console\OutputStyle
, which doesn't implementConsoleOutputInterface
, meaning we won't use STDERR with Laravel even when STDOUT is redirected elsewhere.Laravel's
OutputStyle
class is an extension of Symfony'sSymfonyStyle
andOutputStyle
classes, which are a wrapper around an underlyingOutputInterface
. It doesn't expose thegetErrorOutput
method but instead exposes agetErrorStyle
method. Unfortunately, this returns a new instance ofSymfonyStyle
rather than Laravel'sOutputStyle
class (due to their use ofself
).Even if we could get it to return a new instance of Laravel's
OutputStyle
, the newline tracking would break because Laravel will still be using its originalOutputStyle
instance rather than the new instance we'd be using in Prompts.It's a complicated problem because we need to keep track of the newlines between Prompts's and Laravel's output. Both Laravel and Prompts need to know how many trailing newlines were in the previous output, regardless of where that previous output came from, so that they each know how many newlines to emit before any new output. If Laravel isn't aware of the trailing newlines that Prompts has written, it won't be able to output the correct amount of leading newlines before any new output, and vice versa. It gets more complicated with Symfony because they have a separate instance for STDERR. If Laravel and Prompts use different instances, they won't be aware of each other's trailing newlines.
The only solutions I can think of are:
OutputStyle
class (or create a new wrapper class) that allows us to write to STDERR, while still tracking trailing newlines regardless of the stream. It would potentially need to be smart enough to track newlines separately when streams are redirected to different places because, in that scenario, the trailing newlines from the previous output of another stream shouldn't be considered. Prompts could then choose to output to STDERR and both Laravel and Prompts would still know how many leading newlines they needed to output.Either way it's a big change covering two code bases. I still see the value in doing this, but I'm concerned about the effort and risk involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed explanation. I can't think of any other solutions, so you're welcome to close this if you want.
I learned a lot about console output, and it was nice to meet you guys 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @mortenscheel! Nice to meet you as well 🙂
Happy to reconsider this at some point if we can figure out a good solution. Having one blank line between output looks great, but it certainly adds a lot of challenges 😅