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

Compact sequences like batches #958

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jdhenke
Copy link

@jdhenke jdhenke commented Mar 16, 2024

This change makes Sequence behave like Batch in how it handles nil entries, preventing a possible infinite loop.

Background

I stumbled across this discrepancy when I noticed a bubbletea program I had written had CPU usage over 100% all the time, when I would expect it near zero as a CLI simply waiting for input.

I created a simplified repro, and noticed that swapping in tea.Batch for tea.Sequence fixed the CPU usage, which was nice, but it seems like either one should work.

Digging in, it looks like Batch already had logic to handle all nils in #217 and more recently to return the only non-nil entry directly if possible in #875. This change reuses the same logic (and tests) for Sequence as well now, which avoids creating this infinite loop when using Sequence 👍

Demo of Fix

Here's a walk through showing the fix in action within a go workspace.

$ cat go.work
go 1.21

toolchain go1.21.1

use (
	./bubbletea
	./repro
)

Here's showing the CPU issue before:

image image

And here's showing it's fixed using this branch:

image image

Let me know your thoughts and if you need this PR adjusted in any way, cheers.

@aymanbagabas aymanbagabas deleted the branch charmbracelet:main October 28, 2024 17:41
@aymanbagabas aymanbagabas reopened this Oct 28, 2024
@aymanbagabas aymanbagabas changed the base branch from master to main October 28, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants