Conversation
godoc reference preview ready ✨
|
There was a problem hiding this comment.
Pull request overview
Fixes the events CLI subcommand flow so commands like acmcsuf-cli events get execute directly instead of dropping into an interactive menu, aligning behavior with the pre-“huh forms” CLI.
Changes:
- Replaces the interactive
eventsmenu with a standardeventsroot command and direct subcommands. - Refactors
eventsCRUD commands to use the sharedinternal/cli/clientrequest helper + JSON pretty-printing. - Splits time-related helpers out of the removed
utils/cli_helper.gointoutils/time.go.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
internal/cli/root.go |
Adds a global /health connectivity check in PersistentPreRunE. |
internal/cli/client/client.go |
Adds CheckConnection helper for health checks. |
internal/cli/events/root.go |
Introduces a proper events root command and attaches subcommands. |
internal/cli/events/get.go |
Simplifies get to run directly (optional --id). |
internal/cli/events/delete.go |
Simplifies delete to require --id and run directly. |
internal/cli/events/post.go |
Reworks post to use a huh form + shared HTTP client helper. |
internal/cli/events/put.go |
Reworks put to require --id, fetch old payload, and submit updates via shared client helper. |
internal/cli/events/events.go |
Removes the old interactive menu implementation. |
utils/cli_helper.go |
Deletes the old CLI helper grab-bag file (printing/forms/connection/etc). |
utils/time.go |
Adds time formatting + duration-to-end-time helper (moved from deleted helper file). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| huh.NewInput(). | ||
| Title("Starts At\n"+ | ||
| "Format: \x1b[93mMM/DD/YY HH:MM[PM | AM]\x1b[0m\n"+ | ||
| "Leave empty to keep existing value"). | ||
| Value(&startAtStr), | ||
| huh.NewInput(). | ||
| Title("Ends At\n"+ | ||
| "Format: \x1b[93mMM/DD/YY HH:MM[PM | AM]\x1b[0m\n"+ | ||
| "Leave empty to keep existing value"). | ||
| Value(&endAtStr), |
There was a problem hiding this comment.
The form says "Leave empty to keep existing value" for Starts At/Ends At, but the existing timestamps are neither shown nor pre-filled, so users can't easily tell what value will be kept. Consider pre-populating these fields with the current value (formatted) or including the current value in the prompt text.
| // For unix times of int64 to readable format of 03:04:05PM 01/02/06 | ||
| func FormatUnix(unixTime int64) string { | ||
| t := time.Unix(unixTime, 0) | ||
| return t.Format("01/02/06 03:04PM") | ||
| } |
There was a problem hiding this comment.
FormatUnix's comment says the output includes seconds ("03:04:05PM"), but the layout string formats only hours/minutes ("03:04PM"). Update the comment or the format string so they match.
| // Since I cant go from 03:04:05 -> time.Time directly, I am left doing some... parsing | ||
| re := regexp.MustCompile(`(\d{2}):(\d{2})`) | ||
| parsedDuration := re.FindStringSubmatch(duration) | ||
|
|
||
| if parsedDuration == nil { | ||
| return -1, fmt.Errorf("error, duration time must be in the format: 03:04") | ||
| } |
There was a problem hiding this comment.
TimeAfterDuration uses an unanchored regex (�d{2}):(�d{2}), so strings like "abc01:30xyz" will be accepted, and values like 99:99 will also parse without error. Consider anchoring the match (^...$) and validating hour/minute ranges before computing the duration.
| return fmt.Errorf("failed to load config: %w", err) | ||
| } | ||
| return nil | ||
|
|
There was a problem hiding this comment.
rootCmd.PersistentPreRunE performs a /health check for every command invocation. This will also run for commands like config init and even --help, causing those commands to fail when the API server is down (or blocking on network). Consider skipping the health check for non-API commands (e.g., config, help, completion) or moving the connection check into only the command groups that actually talk to the API.
| // Skip health check for commands that don't require API access | |
| for c := cmd; c != nil; c = c.Parent() { | |
| switch c.Name() { | |
| case "config", "help", "completion": | |
| return nil | |
| } | |
| } |
| func CheckConnection(url string) error { | ||
| _, err := http.Get(url) | ||
| if err != nil { | ||
| // fang/lipgloss lowk nukes this custom formatting | ||
| return fmt.Errorf("\x1b[1;37;41mUNABLE TO CONNECT\x1b[0m | %s\n\t↳ %v", | ||
| "Did you forget to start the server?", | ||
| err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
CheckConnection uses http.Get with the default client (no timeout). Since this is called during CLI startup now, a stalled network connection can hang the CLI indefinitely. Consider using an http.Client with a short timeout (and possibly HEAD instead of GET) to keep the CLI responsive.
| PutEvents.Flags().BoolP("isallday", "a", false, "Set if new event is all day") | ||
|
|
||
| // This flag is neccessary | ||
| PutEvents.Flags().String("id", "", "Get an event by its id") |
There was a problem hiding this comment.
The --id flag help text says "Get an event by its id", but this command is events put (update). Updating the flag description will make --help output accurate.
| PutEvents.Flags().String("id", "", "Get an event by its id") | |
| PutEvents.Flags().String("id", "", "ID of the event to update") |
closes #151