Arbitrary start - #17
Conversation
There was a problem hiding this comment.
Hey @nsilveri - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| def set_minutes_start_time(self, minutes): | ||
| """Set the start time manually in minutes.""" | ||
| self._time_started = time.time() - (minutes * 60) |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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.'
| def set_seconds_start_time(self, seconds): | ||
| """Set the start time manually in minutes.""" | ||
| self._time_started = time.time() - seconds |
There was a problem hiding this comment.
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_started = self._time_started + pause_duration | ||
| self._paused = False | ||
| def set_minutes_start_time(self, minutes): |
There was a problem hiding this comment.
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.
| defset_minutes_start_time(self, minutes): | |
| defset_start_time(self, minutes=0, seconds=0): | |
| """Set the start time manually in minutes and seconds.""" | |
| self._time_started=time.time() - (minutes*60+seconds) |
| start(): start the timer | ||
| pause(): pause the timer | ||
| resume(): resume the timer | ||
| set_minutes_start_time(minutes): set the start time manually in minutes |
There was a problem hiding this comment.
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.
| set_minutes_start_time(minutes): setthestarttimemanuallyinminutes | |
| set_minutes_start_time(minutes): setthestarttimemanuallyinminutes (negativevaluesnotallowed) |
I added the ability to set the minutes or seconds from which to start the timer in the case of a count-up.
use:
counter.set_minutes_start_time(minutes)
counter.start()
For the sake of speed, I reused the pause() function, so it's as if a resume() is executed when .start() is called.
Manual minutes set:
The timer starts from the set minutes
