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

Statusbar to use _closed_time for elapsed when closed #67

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

Conversation

guano
Copy link

@guano guano commented Jun 5, 2024

A preliminary fix for issue #66 , at least it fixes my symptoms.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

We've reviewed this pull request using the Sourcery rules engine. If you would also like our AI-powered code review then let us know.

Copy link
Contributor

@avylove avylove left a comment

Choose a reason for hiding this comment

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

I'm not sure about this approach. The idea here is the clock for status bars is stopped explicitly, where the clock for progress bars is implicit (the last time the count was updated). That might make sense, but I'd need to think about different use cases.

@@ -163,7 +163,7 @@ class PrintableCounter(BaseCounter): # pylint: disable=too-many-instance-attrib
Base class for printable counters
"""

__slots__ = ('_closed', '_count_updated', 'enabled', '_fill', 'last_update',
__slots__ = ('_closed', '_closed_time', '_count_updated', 'enabled', '_fill', 'last_update',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hesitant to add another variable when _count_updated is already available. It could be renamed to be more general purpose.

@@ -180,6 +180,7 @@ def __init__(self, keywords=None, **kwargs):
self.min_delta = kwargs.pop('min_delta', 0.1)
self._pinned = False
self.last_update = self.start = self._count_updated = time.time()
self._closed_time = None
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that the type changes here. If it's going to be a float, it should probably start with the value 0.0.

@@ -272,6 +273,8 @@ def close(self, clear=False):
elif self in self.manager.counters:
self.refresh()

self._closed_time = time.time() - self.start
Copy link
Contributor

Choose a reason for hiding this comment

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

You're calculating elapsed time here and putting it in the closed time variable

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