Skip to content

Conversation

timreichen
Copy link
Contributor

This PR adds a print() call when the progress bar is initialized (setInterval() only starts printing after the delay) and removes the print() call when it is stopped.

@timreichen timreichen requested a review from kt3k as a code owner May 24, 2025 09:07
@github-actions github-actions bot added the cli label May 24, 2025
Copy link

codecov bot commented May 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.70%. Comparing base (5e5d251) to head (385b11d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6676      +/-   ##
==========================================
- Coverage   94.70%   94.70%   -0.01%     
==========================================
  Files         563      563              
  Lines       46681    46689       +8     
  Branches     6571     6575       +4     
==========================================
+ Hits        44209    44216       +7     
- Misses       2430     2431       +1     
  Partials       42       42              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kt3k
Copy link
Member

kt3k commented May 26, 2025

I'm in favor of adding the initial print call, but I don't see why the last print call should be removed. Isn't that also good for showing up-to-date state to the user?

@timreichen
Copy link
Contributor Author

I'm in favor of adding the initial print call, but I don't see why the last print call should be removed. Isn't that also good for showing up-to-date state to the user?

There are two scenarios:

  1. The line gets cleared which makes the print obsolete.
  2. The line stays and a newline gets appended. I think it is reasonable to assume that the current print is kept when calling stop(). With refactor(cli/unstable): print progress bar when value and max are changed #6664 (review), the state gets updated more regularly anyway.

@kt3k
Copy link
Member

kt3k commented May 26, 2025

The line gets cleared which makes the print obsolete.

Then how about only printing when this.#clear is false?

The line stays and a newline gets appended. I think it is reasonable to assume that the current print is kept when calling stop(). With #6664 (review), the state gets updated more regularly anyway.

There are still some delay between the last value/max assignment and printing of them (and the last printing can be skipped if we stop in the middle of delay). If we print at stop, that can show more correct data.

@timreichen
Copy link
Contributor Author

The line gets cleared which makes the print obsolete.

Then how about only printing when this.#clear is false?

The line stays and a newline gets appended. I think it is reasonable to assume that the current print is kept when calling stop(). With #6664 (review), the state gets updated more regularly anyway.

There are still some delay between the last value/max assignment and printing of them (and the last printing can be skipped if we stop in the middle of delay). If we print at stop, that can show more correct data.

Ok that makes sense, with #6664 (once the issues are solved) the print function is will check whether there is an update and return early if not. I'll update this PR.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

@kt3k kt3k merged commit be50073 into denoland:main May 26, 2025
19 checks passed
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.

2 participants