Add --break option to finish subcommand#36
Conversation
justincampbell
force-pushed
the
add-break-option-to-finish
branch
2 times, most recently
from
445b568 to
2462e8e
Compare
There was a problem hiding this comment.
Pull Request Overview
Adds a --break flag to the finish command that automatically starts a break timer after completing a pomodoro. The implementation includes duration parsing that defaults to minutes when no unit is specified, and refactors duplicate code into a shared utility module.
- Added
--breakflag to finish command with customizable duration - Created shared utility functions for countdown timer and duration parsing
- Eliminated code duplication between break and finish commands
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/util.go | New utility file containing shared wait and parseDurationMinutes functions |
| cmd/finish.go | Added --break flag implementation and break logic after finishing pomodoro |
| cmd/break.go | Removed duplicate utility functions that were moved to util.go |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cmd/finish.go
Outdated
| if cmd.Flags().Changed("break") { | ||
| breakDuration := settings.DefaultBreakDuration | ||
|
|
||
| if breakFlag != "" { |
There was a problem hiding this comment.
The logic checks both cmd.Flags().Changed(\"break\") and breakFlag != \"\" which creates redundant conditions. When a flag is changed, the value will either be the provided value or empty string. Consider simplifying to just check if the flag was changed and handle the empty string case as the default duration.
This adds a --break flag to the finish command that allows users to automatically start a break after finishing a pomodoro. The break duration can be specified in minutes. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Remove duplicated wait() and parseDurationMinutes() functions from finish.go and reuse the existing functions from break.go. This eliminates code duplication while maintaining the same functionality. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Moved wait() and parseDurationMinutes() functions from break.go to a new util.go file to better organize shared functionality used by both break and finish commands. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixed logic error where breakFlag was checked twice unnecessarily. Now properly parses the break duration when --break flag is provided. Usage: `pomodoro finish --break 5` (not `pomodoro finish 5`) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…alue When using `pomodoro finish --break` without specifying a duration, the command now properly uses the user's DefaultBreakDuration setting instead of requiring a value. This allows users to customize their default break duration in settings. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
justincampbell
force-pushed
the
add-break-option-to-finish
branch
from
2462e8e to
1e0ae65
Compare
- Add bats tests for --break flag with and without duration - Add Go unit tests for parseDurationMinutes using testify - Fix missing trailing newline in cmd/util.go - Add short flag -b for --break option - Use TrimSpace to properly handle NoOptDefVal 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| command.Flags().StringVarP(&breakFlag, "break", "b", "", "take a break after finishing (duration in minutes)") | ||
| command.Flags().Lookup("break").NoOptDefVal = " " |
There was a problem hiding this comment.
Using a space character as the NoOptDefVal is unclear and could be confusing. Consider using an empty string or a more explicit default value like "default" to make the intent clearer.
| command.Flags().Lookup("break").NoOptDefVal = " " | |
| command.Flags().Lookup("break").NoOptDefVal = "" |
| trimmedFlag := strings.TrimSpace(breakFlag) | ||
| if trimmedFlag != "" { |
There was a problem hiding this comment.
The logic for handling the space character NoOptDefVal is indirect and hard to follow. Since you're trimming spaces and checking for emptiness, the space character from NoOptDefVal will always result in using the default duration, making this approach unnecessarily complex.
Summary
• Added a
--breakflag to thefinishcommand that automatically starts a break after finishing a pomodoro• The break duration can be specified in minutes (e.g.,
--break 5or--break 5m)• Implemented in two separate commits: first working implementation, then refactored to eliminate code duplication
Test plan
pomodoro finish --helpshows the new--breakflagpomodoro finish --break 5starts a 5-minute break after finishing a pomodoro--break🤖 Generated with Claude Code