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

add (and default to on) --preview #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tbg
Copy link
Collaborator

@tbg tbg commented Jan 14, 2025

demo

With this flag, the benchdiff output is rendered between each
interleaved run of both the old and new benches (within the current
package).

Especially for longer benchmark runs, this is helpful to give an idea
of what the results are likely going to be. In some cases, this may
prompt the user to call it early, thus saving time. But it's also
pleasant to be able to just check in, which is my primary motivation
for this change.

tbg added 2 commits January 14, 2025 13:39
With this flag, the benchdiff output is rendered between each
interleaved run of both the old and new benches (within the current
package).

Especially for longer benchmark runs, this is helpful to give an idea
of what the results are likely going to be. In some cases, this may
prompt the user to call it early, thus saving time. But it's also
pleasant to be able to just check in, which is my primary motivation
for this change.
@tbg tbg changed the title incprgr add (and default to on) --preview Jan 14, 2025
@tbg tbg requested a review from nvanbenschoten January 14, 2025 12:46
Copy link

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Nice!

spinner.Update(progress)
var buf bytes.Buffer
if preview {
_, err := processBenchOutput(ctx, &buf, bs1, bs2, true, text, tests, nil)

Choose a reason for hiding this comment

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

[nit] add /* byName */

Choose a reason for hiding this comment

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

Should we skip this when j==0? Not sure what it would show (maybe just the header columns?)

if err != nil {
return err
}
_, _ = fmt.Fprintln(&buf)

Choose a reason for hiding this comment

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

[supernit] There's an extra space because of how we're using the spinner. Maybe add a \n at the beginning of buf so it doesn't matter. Or change the spinner to not add the space if prefix is empty and use an empty prefix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants