refactor: migrate away from cheggaaa/pb v1#11322
Merged
Merged
Conversation
Contributor
Author
|
@lidel Hi! Could I get a review on this PR when you have a chance? Thanks! :) |
Contributor
Author
|
Fixed the issue and pushed the changes. |
Member
|
Thank you for taking a stab at this @Vinayak9769! Would be good to discuss it first in #11316 (pros and cons of this vs other libraries), but since you already did the work this is useful as a data point, thanks! |
gammazero
added
the
status/blocked
Replace three duplicate TTY checks (get.go, dag/export.go, dag/stat.go)
with `cmdenv.IsTerminal(*os.File)` backed by `mattn/go-isatty`.
The helper uses `IsTerminal || IsCygwinTerminal`, which also detects
MSYS2 and Git Bash on Windows. Those terminals expose stdio as a
named pipe rather than a character device, so the previous
`ModeCharDevice` check suppressed the progress bar on real terminals.
- core/commands/cmdenv/tty.go: new helper
- core/commands/{add,cat,get}.go: drop local isStderrTTY
- core/commands/dag/{export,stat}.go: drop inline stat() block
- go.mod: promote mattn/go-isatty to direct (was indirect via pb/v3)
Collapse the explicit-flag-or-TTY-default logic at four call sites (`cat`, `get`, `dag export`, `dag stat`) into a single helper.
The full bar template (counters, bar, speed, percent, ETA) was inlined at two call sites in add.go. Move it to a file-level const.
# Conflicts: # go.mod # go.sum
pb v3's speed element defaults to suffix "%s p/s", so even with pb.Bytes set, `ipfs add`, `ipfs cat`, `ipfs get`, and `ipfs dag export` rendered the rate as "713.04 MiB p/s" instead of "713.04 MiB/s". Pass explicit format args to the speed and rtime template elements: rate now renders as "MiB/s", and the unknown-state fallback reads "?/s" / "ETA ?" instead of bare "?". The four templates move to package-level consts.
Describe only the user-visible changes; skip library-migration detail and intermediate-state claims that never shipped.
Member
|
Pushed a few follow-up commits:
I'll check on CI tomorrow. There is a lot to improve about progress bars, but we would have to do much deeper refactor. This is a good first step. Once CI is green, let's merge as-is and ship in 0.42. |
The `github.com/cheggaaa/pb` (v1) module path is no longer in `go.mod` after the migration to `pb/v3`, so the ignore rule never fires.
Match the wording used by `add`, `cat`, and `get`: "Stream progress data. Defaults to true when stderr is a terminal."
Call `bar.Finish()` and a final `bar.Write()` after the progress loop. Without it, fast adds (under ~500ms, where pb/v3's EWMA never accumulates a speed sample) render `?/s ... ETA ?` in the last frame. Finishing the bar switches the speed element to its absolute-rate branch (total/elapsed), so the final frame now reads e.g. `792.04 MiB/s 100.00% 100ms`.
Exercise the explicit-true, explicit-false, unset, and non-bool paths. Unset and non-bool fall back to IsTerminal(os.Stderr), which the test compares against directly so it works in both TTY and CI environments.
Move the "total known" pb/v3 template to cmdenv.ProgressBarFullTemplate so add.go and get.go reference the same string instead of keeping byte-identical local copies. The add init template and dag/export streaming template stay local because each is single-use and shaped differently.
lidel
approved these changes
May 25, 2026
Member
There was a problem hiding this comment.
A few more follow-ups on top:
ipfs add: bar now finalizes properly so the last frame doesn't stick mid-render on fast uploads- shared
cmdenv.ProgressBarFullTemplatebetweenipfs addandipfs getto drop a duplicated template - unit test for
ShouldShowProgress --progresshelp text unified across commands- dropped unused pb v1 entry from
.github/dependabot.yml
Planning to cut v0.42.0-rc1 with this later this/next week.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This PR migrates all progress bar usage from
github.com/cheggaaa/pbv1 togithub.com/cheggaaa/pb/v3and aligns progress auto-detection behavior acrosscommands.
Closes #11316
Changes
github.com/cheggaaa/pb/v3.github/dependabot.ymlto ignore the obsolete v1 module pathisStderrTTY()handling forcat,get, andaddcat,get,add, anddag exportconsistently default progress toenabled only when stderr is a terminal