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.

Sign up for GitHub

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

Arbitrary start #17

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

Arbitrary start #17

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bff76bb
Adding the possibility to set the initial min/sec from which to start.
nsilveri May 28, 2024
7248ca6
Adding the possibility to set the initial min/sec from which to start.
nsilveri May 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions count_timer/count_timer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ class CountTimer:
start(): start the timer
pause(): pause the timer
resume(): resume the timer
set_minutes_start_time(minutes): set the start time manually in minutes
Copy link

Choose a reason for hiding this comment

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

suggestion: Consider adding validation details in the docstring.

It would be helpful to mention in the docstring that negative values for minutes and seconds are not allowed, assuming you add validation for this.

Suggested change
set_minutes_start_time(minutes): set the start time manually in minutes
set_minutes_start_time(minutes): set the start time manually in minutes (negative values not allowed)

set_seconds_start_time(seconds): set the start time manually in seconds
reset(): reset the timer to default (duration 0/paused/not started)

Properties:
Expand Down Expand Up @@ -63,6 +65,18 @@ def resume(self):
self._time_started = self._time_started + pause_duration
self._paused = False

def set_minutes_start_time(self, minutes):
Copy link

Choose a reason for hiding this comment

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

suggestion: Consider combining set_minutes_start_time and set_seconds_start_time.

Both methods are very similar. You could create a single method that takes both minutes and seconds as optional parameters, defaulting to zero, to reduce code duplication.

Suggested change
def set_minutes_start_time(self, minutes):
def set_start_time(self, minutes=0, seconds=0):
"""Set the start time manually in minutes and seconds."""
self._time_started = time.time() - (minutes * 60 + seconds)

"""Set the start time manually in minutes."""
self._time_started = time.time() - (minutes * 60)
Copy link

Choose a reason for hiding this comment

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

issue (bug_risk): Potential issue with negative time values.

If a negative value is passed to minutes, it will set _time_started to a future time. Consider adding validation to ensure minutes is non-negative.

self._time_paused = time.time()
self._paused = True

def set_seconds_start_time(self, seconds):
Copy link

Choose a reason for hiding this comment

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

issue (typo): Incorrect docstring for set_seconds_start_time.

The docstring for set_seconds_start_time incorrectly states 'Set the start time manually in minutes.' It should be 'Set the start time manually in seconds.'

"""Set the start time manually in minutes."""
self._time_started = time.time() - seconds
Copy link

Choose a reason for hiding this comment

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

issue (bug_risk): Potential issue with negative time values.

If a negative value is passed to seconds, it will set _time_started to a future time. Consider adding validation to ensure seconds is non-negative.

self._time_paused = time.time()
self._paused = True

def _get(self) -> float:
"""Time in sec since timer was started, minus any time paused."""
if not self._time_started:
Expand Down