Skip to content

feat: command - add support for typed cli arguments #1199

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

Merged
merged 10 commits into from
Sep 19, 2025
Merged

feat: command - add support for typed cli arguments #1199

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9185f34
feat: command - add support for typed cli arguments
oprudkyi Sep 4, 2025
211794f
chore: update mocks
oprudkyi Sep 14, 2025
e8d5afa
feat: tests for Argument..() functions
oprudkyi Sep 14, 2025
47604ec
fix: rename argument types to have Argument word first
oprudkyi Sep 16, 2025
687e51d
fix: move TestCliContextArguments into cli_context_test.go
oprudkyi Sep 16, 2025
6559400
feat: gracefully handle argument registration errors
oprudkyi Sep 17, 2025
5b57460
feat: move errors into errors/list.go
oprudkyi Sep 17, 2025
d79c7ce
chore: update mocks
oprudkyi Sep 17, 2025
66afa01
fix: don't register command on error
oprudkyi Sep 18, 2025
72910e9
feat: support time layouts in ArgumentTimestamp{Slice}
oprudkyi Sep 18, 2025
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
262 changes: 262 additions & 0 deletions console/application.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import (
"os"
"slices"
"strings"
"time"

"github.com/urfave/cli/v3"

"github.com/goravel/framework/contracts/console"
"github.com/goravel/framework/contracts/console/command"
"github.com/goravel/framework/errors"
"github.com/goravel/framework/support/color"
"github.com/goravel/framework/support/env"
)
Expand Down Expand Up @@ -50,6 +52,10 @@ func NewApplication(name, usage, usageText, version string, useArtisan bool) con
func (r *Application) Register(commands []console.Command) {
for _, item := range commands {
item := item
arguments, err := argumentsToCliArgs(item.Extend().Arguments)
if err != nil {
color.Errorln(errors.ConsoleCommandRegistrationFailed.Args(item.Signature(), err).Error())
Copy link
Contributor

@almas-x almas-x Sep 17, 2025

Choose a reason for hiding this comment

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

After printing the error here, it might be better to use continue to skip the iteration, right?

Copy link
Contributor Author

@oprudkyi oprudkyi Sep 18, 2025

Choose a reason for hiding this comment

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

added continue

I did it without continue because for me it provides at least some context for user

./artisan smtp:add  --help      
  ERROR   Registration of command 'smtp:add' failed: required argument 'host' should be placed before any not-required arguments
Description:
   Add SMTP server

   Stores SMTP server by provided server_id into database


Usage:
   artisan [global options] smtp:add [options]

with continue

./artisan smtp:add  --help
  ERROR   Registration of command 'smtp:add' failed: required argument 'host' should be placed before any not-required arguments
  ERROR   Command 'smtp:add' is not defined. Did you mean one of these?

}
cliCommand := cli.Command{
Name: item.Signature(),
Usage: item.Description(),
Expand All @@ -59,6 +65,7 @@ func (r *Application) Register(commands []console.Command) {
Category: item.Extend().Category,
ArgsUsage: item.Extend().ArgsUsage,
Flags: flagsToCliFlags(item.Extend().Flags),
Arguments: arguments,
OnUsageError: onUsageError,
}
r.instance.Commands = append(r.instance.Commands, &cliCommand)
Expand Down Expand Up @@ -229,3 +236,258 @@ func flagsToCliFlags(flags []command.Flag) []cli.Flag {

return cliFlags
}

func argumentsToCliArgs(args []command.Argument) ([]cli.Argument, error) {
len := len(args)
if len == 0 {
return nil, nil
}
cliArgs := make([]cli.Argument, 0, len)
previousIsRequired := true
for _, v := range args {
if v.MinOccurrences() != 0 && !previousIsRequired {
return nil, errors.ConsoleCommandRequiredArgumentWrongOrder.Args(v.ArgumentName())
}
if v.MinOccurrences() != 0 {
previousIsRequired = true
} else {
previousIsRequired = false
Comment on lines +246 to +254
Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

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

[nitpick] The logic for tracking previousIsRequired can be simplified. The variable should be set to false when encountering the first non-required argument, and the validation should check if any required arguments come after that point. Consider refactoring to use a single boolean flag hasNonRequiredArgument for clearer intent.

Suggested change
previousIsRequired := true
for _, v := range args {
if v.MinOccurrences() != 0 && !previousIsRequired {
return nil, errors.ConsoleCommandRequiredArgumentWrongOrder.Args(v.ArgumentName())
}
if v.MinOccurrences() != 0 {
previousIsRequired = true
} else {
previousIsRequired = false
hasNonRequiredArgument := false
for _, v := range args {
if hasNonRequiredArgument && v.MinOccurrences() != 0 {
return nil, errors.ConsoleCommandRequiredArgumentWrongOrder.Args(v.ArgumentName())
}
if v.MinOccurrences() == 0 {
hasNonRequiredArgument = true

Copilot uses AI. Check for mistakes.
}
switch arg := v.(type) {
case *command.ArgumentFloat32:
cliArgs = append(cliArgs, &cli.Float32Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentFloat64:
cliArgs = append(cliArgs, &cli.Float64Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentInt:
cliArgs = append(cliArgs, &cli.IntArgs{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentInt8:
cliArgs = append(cliArgs, &cli.Int8Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentInt16:
cliArgs = append(cliArgs, &cli.Int16Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentInt32:
cliArgs = append(cliArgs, &cli.Int32Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentInt64:
cliArgs = append(cliArgs, &cli.Int64Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentString:
cliArgs = append(cliArgs, &cli.StringArgs{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentTimestamp:
cliArgs = append(cliArgs, &cli.TimestampArgs{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
Config: cli.TimestampConfig{
Layouts: []string{time.RFC3339},
Copy link
Contributor

@almas-x almas-x Sep 17, 2025

Choose a reason for hiding this comment

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

How about adding a time format configuration option to command.ArgumentTimestamp? This would allow users to specify their desired format, making it more flexible and user-friendly.

Copy link
Contributor Author

@oprudkyi oprudkyi Sep 18, 2025

Choose a reason for hiding this comment

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

added support for Layouts

},
})
case *command.ArgumentUint:
cliArgs = append(cliArgs, &cli.UintArgs{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentUint8:
cliArgs = append(cliArgs, &cli.Uint8Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentUint16:
cliArgs = append(cliArgs, &cli.Uint16Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentUint32:
cliArgs = append(cliArgs, &cli.Uint32Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentUint64:
cliArgs = append(cliArgs, &cli.Uint64Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})

case *command.ArgumentFloat32Slice:
cliArgs = append(cliArgs, &cli.Float32Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentFloat64Slice:
cliArgs = append(cliArgs, &cli.Float64Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentIntSlice:
cliArgs = append(cliArgs, &cli.IntArgs{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentInt8Slice:
cliArgs = append(cliArgs, &cli.Int8Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentInt16Slice:
cliArgs = append(cliArgs, &cli.Int16Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentInt32Slice:
cliArgs = append(cliArgs, &cli.Int32Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentInt64Slice:
cliArgs = append(cliArgs, &cli.Int64Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentStringSlice:
cliArgs = append(cliArgs, &cli.StringArgs{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentTimestampSlice:
cliArgs = append(cliArgs, &cli.TimestampArgs{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
Config: cli.TimestampConfig{
Layouts: []string{time.RFC3339},
Copy link
Contributor

@almas-x almas-x Sep 17, 2025

Choose a reason for hiding this comment

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

Ditto

},
})
case *command.ArgumentUintSlice:
cliArgs = append(cliArgs, &cli.UintArgs{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentUint8Slice:
cliArgs = append(cliArgs, &cli.Uint8Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentUint16Slice:
cliArgs = append(cliArgs, &cli.Uint16Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentUint32Slice:
cliArgs = append(cliArgs, &cli.Uint32Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
case *command.ArgumentUint64Slice:
cliArgs = append(cliArgs, &cli.Uint64Args{
Name: arg.Name,
UsageText: arg.Usage,
Value: arg.Value,
Min: arg.MinOccurrences(),
Max: arg.MaxOccurrences(),
})
default:
return nil, errors.ConsoleCommandArgumentUnknownType.Args(arg, arg)
Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

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

The error formatting passes the same argument twice. The second parameter should be the argument's value or a different property. This will result in redundant information in the error message.

Copilot uses AI. Check for mistakes.
}
}
return cliArgs, nil
}
Loading
Loading