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

feat: Wrap error returned from run with context error #1215

Closed
wants to merge 4 commits into from
Closed

feat: Wrap error returned from run with context error #1215

wants to merge 4 commits into from

Conversation

goodevilgenius
Copy link

@goodevilgenius goodevilgenius commented Oct 29, 2024 •
edited
Loading

Fixes #1212

This change allows the error returned by p.Run to be checked for a context error.

For example, given this code:

ctx := context.WithDeadline(context.Background, time.Second*5)
p := tea.NewProgram(model, tea.WithContext(ctx))
_, err := p.Run()
if errors.Is(err, context.DeadlineExceeded) {
  // Handle timeout
}

This change now allows this to work.

This opens up more possibilities, allowing a program to cancel a context, and look for the reason for the cancellation.

@goodevilgenius
Copy link
Author

Ooh, I see go.mod is pinned to version 1.18. That makes context.Cause unavailable. That's unfortunate. I'll revert that commit, so it's compatible with 1.18.

@goodevilgenius
revert: feat: use context.Cause to get context error
92fe748 
go1.18 doesn't have context.Cause

This reverts commit 3161ec2.
@meowgorithm
Copy link
Member

Wow, that was fast! If functionality is the same with the new version, want to also put a little comment in there about how to use the hot, new context.Cause when we're ready to bump the lib up to 1.20?

@goodevilgenius
Copy link
Author

@meowgorithm Sure. Not a problem. What's the standard way to do that on this project? A // TODO comment above the code, or some other method?

@meowgorithm
Copy link
Member

// TODO: is just fine!

meowgorithm
meowgorithm reviewed Oct 29, 2024
View reviewed changes
tea.go
@@ -595,7 +595,7 @@ func (p *Program) Run() (Model, error) {
model, err := p.eventLoop(model, cmds)
killed := p.ctx.Err() != nil
if killed {
err = fmt.Errorf("%w: %s", ErrProgramKilled, p.ctx.Err())
err = fmt.Errorf("%w: %w", ErrProgramKilled, p.ctx.Err())
Copy link
Member

Choose a reason for hiding this comment

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

Wow, TIL you cannot double-wrap

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it does something similar to errors.Join when you double wrap errors. This does break errors.Unwrap, since that function only works on singly-wrapped errors, but errors.Is and errors.As continues to work as expected.

@goodevilgenius
docs: Add TODO for 1.20 upgrade
e9322f2 
Ensure we use context.Cause when it becomes available
@goodevilgenius
Copy link
Author

Darnit. Looks like this is not possible with this version of go. Pipeline is still failing with:

fmt.Errorf call has more than one error-wrapping directive %w

It works fine on my machine, running 1.23.

I checked back with the old docs. fmt.Errorf says this for 1.18:

It is invalid to include more than one %w verb

But, for 1.20 and after, it says:

If there is more than one %w verb, the returned error will implement an Unwrap method returning a []error containing all the %w operands in the order they appear in the arguments.

Looks like this is a non-starter as long as bubbletea is based on 1.18.

I see that the 2.0 alpha branch is still on 1.18, and not a newer version. Any thoughts on when this upgrade might happen?

@meowgorithm
Copy link
Member

Maybe it's time. I know a lot of projects typically support the last two versions of Go, and 1.18 came out about two and a half years ago. Want to reopen this?

@goodevilgenius
Copy link
Author

@meowgorithm Should I reopen it targeting the 2.0 branch, instead of main?
I imagine bumping the version of go would probably constitute a breaking change, no?

@meowgorithm
Copy link
Member

v2 would be ideal, however are you okay working against that version, seeing that it will take some time for us to reach a stable release?

@goodevilgenius
Copy link
Author

I don't need this feature in my project yet (just a small personal thing), so I'm cool adding this to v2. I'll reopen against that.

@meowgorithm
Copy link
Member

Sounds good. This will be a good one to get in, so thank you.

@goodevilgenius
Copy link
Author

I'll rebase against the v2 branch, and open a new one. I can't find a way to change the base on GitHub anyway.

@goodevilgenius goodevilgenius mentioned this pull request Oct 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meowgorithm meowgorithm meowgorithm left review comments

@aymanbagabas aymanbagabas Awaiting requested review from aymanbagabas aymanbagabas is a code owner

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

Successfully merging this pull request may close these issues.

context error is swallowed up, preventing errors.Is
2 participants
@goodevilgenius @meowgorithm