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

oprudkyi · 2025-09-14T17:07:51Z

📑 Description

adds support for https://cli.urfave.org/v3/examples/arguments/advanced/
--help prints arguments according to https://pubs.opengroup.org/onlinepubs/9799919799/
currently it follows Flag/Options naming pattern , i.e.

	return command.Extend{
		Arguments: []command.Argument{
			&command.StringArgument{
				Name:     "my_string",
				Usage:    "string argument",
				Required: true,
			},
....
	ctx.Info(fmt.Sprintf("my string: %v", ctx.ArgumentString("my_string")))

but what confuses me, is inconsistency in naming, i.e. type StringArgument but method ArgumentString(),
probably type also should be named ArgumentString ?

(probably the same should be done for Flags as well, BoolFlag vs OptionBool())

also there are changes I am not sure if they are fine globally

without this I see error message with pointer, not text

func printTemplateError(err error) {
	if os.Getenv("CLI_TEMPLATE_ERROR_DEBUG") != "" {
-		_, _ = fmt.Fprintf(cli.ErrWriter, "CLI TEMPLATE ERROR: %#v\n", err)
+		_, _ = fmt.Fprintf(cli.ErrWriter, "CLI TEMPLATE ERROR: %+v\n", err)
	}
}

making things testable but just exit with on error

+			errString := fmt.Sprintf("Required argument with value '%+v' should be placed before any not-required arguments", v)
+			if testing.Testing() {
+				panic(errString)
+			} else {
+				color.Errorln(errString)
+				os.Exit(2)
+			}

✅ Checks

Added test cases for my code

codecov · 2025-09-14T17:15:21Z

Codecov Report

❌ Patch coverage is 98.42932% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.73%. Comparing base (15cc675) to head (72910e9).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
console/application.go	98.03%	4 Missing and 1 partial ⚠️
console/cli_helper.go	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1199      +/-   ##
==========================================
+ Coverage   68.55%   68.73%   +0.18%     
==========================================
  Files         228      232       +4     
  Lines       14588    15102     +514     
==========================================
+ Hits        10001    10381     +380     
- Misses       4229     4361     +132     
- Partials      358      360       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- https://cli.urfave.org/v3/examples/arguments/advanced/ - `--help` prints arguments according to https://pubs.opengroup.org/onlinepubs/9799919799/

almas-x · 2025-09-14T22:02:18Z

Amazing👍

almas-x · 2025-09-14T22:05:58Z

console/application.go

+	previousIsRequired := true
+	for _, v := range args {
+		if v.MinOccurrences() != 0 && !previousIsRequired {
+			errString := fmt.Sprintf("Required argument with value '%+v' should be placed before any not-required arguments", v)


It would be better to consolidate all error messages into the existing errors/list.go file.

almas-x · 2025-09-14T22:16:30Z

console/application.go

+			if testing.Testing() {
+				panic(errString)
+			} else {
+				color.Errorln(errString)
+				os.Exit(2)
+			}


I think it would be better to add a check to determine if the environment is an Artisan environment. If it is an Artisan environment, we can output the error message and perform os.Exit. For other cases, such as test environments or non-Artisan execution environments, we can only output the error message without making the argumentor command effective, ensuring that the application as a whole continues to run.

Additionally, it might be worth mentioning that os.Exit can be assigned to a variable, for example, var osExit = os.Exit, so that it can be replaced in unit tests to make the code testable.

I think it would be better to add a check to determine if the environment is an Artisan environment.

I think it should be fine, the application can't be built successfully if there are problems, given that this function is called by Register, and Register is called when initiating the framework.

I think it would be better to add a check to determine if the environment is an Artisan environment.

I think it should be fine, the application can't be built successfully if there are problems, given that this function is called by Register, and Register is called when initiating the framework.

Regarding this point, the error here is due to having a required argument after an optional argument. This is a logical mistake, but it doesn't affect the build process. However, the application will call os.Exit(2) during startup, which is very unfriendly. It's acceptable to interrupt execution when running artisan commands, but if you're simply starting a web service (not running artisan), being forced to exit is terrible. You can refer to how Laravel handles this scenario.

The worst-case scenario is: you install a third-party package, everything works fine, and one day after upgrading, the package introduces a new command. If there is a configuration issue with the command arguments, os.Exit will be called—even if you never use the artisan commands from that package, your application won't be able to start properly.

Good point, so there are two solutions when there is an argument issue:

Exit the app, go build and go run will fail, user needs to fix the argument issue immediately.

Print the error with a graceful message, and the command will not be registered.

The second should be fine.

hwbrzzl

Awesome, thanks! A few of questions.

hwbrzzl · 2025-09-15T09:01:58Z

console/application.go

+			previousIsRequired = false
+		}
+		switch arg := v.(type) {
+		case *command.Float32Argument:


but what confuses me, is inconsistency in naming, i.e. type StringArgument but method ArgumentString(),
probably type also should be named ArgumentString ?

Yes, good point. Keeping consistency should be better: Argument*. By the way, about the Flags option, how about modifying it to Options? Keep it consistent with Option* functions. And type BoolFlag struct, etc. structs can be optimized as well. Adding the deprecated flag for them. cc @almas-x

I'd like to confirm: Are you suggesting merging the existing Flag and the new Argument introduced in this PR into Option? If so, wouldn't this potentially introduce significant breaking changes? It seems that all existing artisan commands would encounter type errors, since the command.Extend method in command definitions currently refers to the xxxFlag structs.

Are you suggesting merging the existing Flag and the new Argument introduced in this PR into Option?

No, I mean copy all flags to options, then add a DEPRECATED flag for flags. There is no breaking change.

Then the logic will be:

Set Options in Extend -> Use ctx.Option* to get data.
Set Arguments in Extend -> Use ctx.Argument* to get data.

We can implement it in another PR if it's acceptable.

hwbrzzl · 2025-09-15T09:10:21Z

console/application.go

+			if testing.Testing() {
+				panic(errString)
+			} else {
+				color.Errorln(errString)
+				os.Exit(2)
+			}


I think it would be better to add a check to determine if the environment is an Artisan environment.

I think it should be fine, the application can't be built successfully if there are problems, given that this function is called by Register, and Register is called when initiating the framework.

hwbrzzl · 2025-09-15T09:13:10Z

console/cli_context_args_test.go

How about merging this file to cli_context_test.go.

hwbrzzl · 2025-09-15T09:14:09Z

console/cli_helper.go

+	argsTemplate  = `{{if .Arguments}}{{range .Arguments}}{{template "argTemplate" .}}{{end}}{{end}}`
+	argTemplate   = ` {{if .Min}}<{{else}}[{{end}}{{.Name}}{{if (or (gt .Max 1) (eq .Max -1))}}...{{end}}{{if .Min}}>{{else}}]{{end}}`


Could you add a screenshot for this printing?

Speaking of help information output, the current implementation uses Go templates, which are honestly very hard to maintain—and debugging is even more troublesome. That's why I've always wanted to refactor this part and directly use the framework's color package for printing. This way, handling command sorting, alignment, and indentation would be much easier. In fact, I even submitted a PR to urfave/cli specifically for this purpose: @urfave/cli/pull/2150.

@hwbrzzl this is my testing command with various cases, required/optional/slice

oprudkyi · 2025-09-15T23:33:38Z

summary for further changes:

It would be better to consolidate all error messages into the existing errors/list.go file. ( @almas-x )
on Register() errors - print the error with a graceful message, and the command will not be registered. (@hwbrzzl)
rename argument types, i.e. Float32Argument to ArgumentFloat32 (@hwbrzzl)
merge console/cli_context_args_test.go into cli_context_test.go (@hwbrzzl)

hwbrzzl · 2025-09-16T01:54:05Z

summary for further changes:

It would be better to consolidate all error messages into the existing errors/list.go file. ( @almas-x )

on Register() errors - print the error with a graceful message, and the command will not be registered. (@hwbrzzl)

rename argument types, i.e. Float32Argument to ArgumentFloat32 (@hwbrzzl)

merge console/cli_context_args_test.go into cli_context_test.go (@hwbrzzl)

Yes, thanks!

oprudkyi · 2025-09-17T12:14:19Z

@hwbrzzl @almas-x
done.
also I am in progress with adding wrappers in onUsageError
but it seems the info about argument isn't provided always , so it is delayed urfave/cli#2203

almas-x · 2025-09-17T12:23:09Z

console/application.go

+				Min:       arg.MinOccurrences(),
+				Max:       arg.MaxOccurrences(),
+				Config: cli.TimestampConfig{
+					Layouts: []string{time.RFC3339},


almas-x · 2025-09-17T12:24:12Z

console/application.go

+				Min:       arg.MinOccurrences(),
+				Max:       arg.MaxOccurrences(),
+				Config: cli.TimestampConfig{
+					Layouts: []string{time.RFC3339},


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.

added support for Layouts

almas-x · 2025-09-17T12:28:49Z

console/application.go

 		item := item
+		arguments, err := argumentsToCliArgs(item.Extend().Arguments)
+		if err != nil {
+			color.Errorln(errors.ConsoleCommandRegistrationFailed.Args(item.Signature(), err).Error())


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

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?

- https://cli.urfave.org/v3/examples/flags/basics/#timestamp-flag - https://pkg.go.dev/time#pkg-constants

Copilot

Pull Request Overview

This PR adds support for typed CLI arguments to the console command system, enabling developers to define and access strongly-typed command arguments beyond the basic string arguments previously available.

Introduces comprehensive argument type definitions (Int, String, Float, Timestamp, and their slice variants)
Adds argument validation with proper error handling for required argument ordering
Implements argument parsing and access methods in the CLI context

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`contracts/console/command/command_args.go`	Defines the Argument interface and all typed argument structs
`contracts/console/command/command.go`	Adds Arguments field to command Extend struct
`contracts/console/command.go`	Adds argument accessor methods to Context interface
`console/application.go`	Implements argument conversion and registration logic
`console/cli_context.go`	Implements argument accessor methods
`console/cli_helper.go`	Updates help templates to display argument information
`errors/list.go`	Adds new error constants for argument validation
`mocks/`	Generated mock files for new interfaces and methods
Test files	Comprehensive test coverage for new argument functionality

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-19T02:12:56Z

console/application.go

+	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


[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 · 2025-09-19T02:12:57Z

console/application.go

+				Max:       arg.MaxOccurrences(),
+			})
+		default:
+			return nil, errors.ConsoleCommandArgumentUnknownType.Args(arg, arg)


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.

hwbrzzl

Awesome, thanks!

oprudkyi requested a review from a team as a code owner September 14, 2025 17:07

oprudkyi force-pushed the issue/cli_arguments branch from 1332987 to 9eae673 Compare September 14, 2025 17:09

oprudkyi added 2 commits September 14, 2025 19:19

feat: command - add support for typed cli arguments

9185f34

- https://cli.urfave.org/v3/examples/arguments/advanced/ - `--help` prints arguments according to https://pubs.opengroup.org/onlinepubs/9799919799/

chore: update mocks

211794f

oprudkyi force-pushed the issue/cli_arguments branch from 87a4783 to 211794f Compare September 14, 2025 17:21

feat: tests for Argument..() functions

e8d5afa

oprudkyi force-pushed the issue/cli_arguments branch from 46452f9 to e8d5afa Compare September 14, 2025 18:05

almas-x reviewed Sep 14, 2025

View reviewed changes

hwbrzzl added the 🚀 Review Ready label Sep 15, 2025

hwbrzzl reviewed Sep 15, 2025

View reviewed changes

oprudkyi added 5 commits September 16, 2025 17:21

fix: rename argument types to have Argument word first

47604ec

fix: move TestCliContextArguments into cli_context_test.go

687e51d

feat: gracefully handle argument registration errors

6559400

feat: move errors into errors/list.go

5b57460

chore: update mocks

d79c7ce

almas-x reviewed Sep 17, 2025

View reviewed changes

oprudkyi added 2 commits September 18, 2025 08:11

fix: don't register command on error

66afa01

feat: support time layouts in ArgumentTimestamp{Slice}

72910e9

- https://cli.urfave.org/v3/examples/flags/basics/#timestamp-flag - https://pkg.go.dev/time#pkg-constants

hwbrzzl requested a review from Copilot September 19, 2025 02:12

Copilot AI reviewed Sep 19, 2025

View reviewed changes

hwbrzzl approved these changes Sep 19, 2025

View reviewed changes

hwbrzzl merged commit f1b75b4 into goravel:master Sep 19, 2025
14 checks passed

oprudkyi mentioned this pull request Sep 26, 2025

feat: documentation for console arguments goravel/docs#132

Merged

		argsTemplate = `{{if .Arguments}}{{range .Arguments}}{{template "argTemplate" .}}{{end}}{{end}}`
		argTemplate = ` {{if .Min}}<{{else}}[{{end}}{{.Name}}{{if (or (gt .Max 1) (eq .Max -1))}}...{{end}}{{if .Min}}>{{else}}]{{end}}`

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

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

Uh oh!

Conversation

oprudkyi commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📑 Description

✅ Checks

Uh oh!

codecov bot commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

almas-x commented Sep 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hwbrzzl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oprudkyi commented Sep 15, 2025

Uh oh!

hwbrzzl commented Sep 16, 2025

Uh oh!

oprudkyi commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

hwbrzzl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oprudkyi commented Sep 14, 2025 •

edited

Loading

codecov bot commented Sep 14, 2025 •

edited

Loading

oprudkyi commented Sep 17, 2025 •

edited

Loading