Skip to content

Conversation

@bhrutledge
Copy link
Contributor

@bhrutledge bhrutledge commented Feb 22, 2022

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review.

Description

While replacing Twine's progress bar with Rich, I got into some bike-shedding/yak-shaving to declutter the time column. After looking at several download progress indicators (e.g. Chrome, Firefox, wget, scp), it seems more common to show time remaining than time elapsed. Both wget and scp show time remaining until the download is finished, then switch to time elapsed. So, I implemented that behavior here.

For example:

twine-progress-rich-time.mp4

rich/progress.py Outdated
return Text(str(remaining_delta), style="progress.remaining")


class CondensedTimeColumn(ProgressColumn):
Copy link
Member

Choose a reason for hiding this comment

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

The naming isn't clear. What is condensed time? How is it different from TimeRemaining or TimeElapsed? I know the answers, but I don't think it would be obvious to a user.

I do like the functionality. But if we add it I think it should be rolled in to the TimeRemainingColumn class so that the defaults continue to work as before, but with options for the new features.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I don't love Condensed, but I chose that thinking of "a shorter time column with more information". Maybe Compact would be clearer.

However, I'm happy to roll it into TimeRemainingColumn. How about:

class TimeRemainingColumn(ProgressColumn):
    """Renders estimated time remaining.

    Args:
        compact (bool, optional): Render MM:SS when time remaining is less than an hour. Defaults to False.
        elapsed_when_finished (bool, optional): Render time elapsed when the task is finished. Defaults to False.
    """

Or, should compact (or another name) encompass both behaviors? Or ???

Copy link
Member

Choose a reason for hiding this comment

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

I like that idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2022

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.81%. Comparing base (73282dd) to head (e2ccd5b).
Report is 1439 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1992   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files          71       71           
  Lines        7045     7055   +10     
=======================================
+ Hits         7032     7042   +10     
  Misses         13       13           
Flag Coverage Δ
unittests 99.81% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@willmcgugan
Copy link
Member

Thanks.

@willmcgugan willmcgugan merged commit 646d933 into Textualize:master Feb 27, 2022
@bhrutledge
Copy link
Contributor Author

My pleasure; thanks for accepting it. Any idea of when it might be released? No pressure, just curious when I can start using this in Twine. 😉

@willmcgugan
Copy link
Member

Just a few days I expect.

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.

3 participants