Skip to content
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.

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

while print flag , the placeholder if need but not set. #2043

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jokemanfire
Copy link
Contributor

What type of PR is this?

  • feature

What this PR does / why we need it:

Change like this:

OPTIONS:
--bar value
--help, -h show help

OPTIONS:
--bar string
--help, -h show help

Special notes for your reviewer:

This is a feature that I like because having a placeholder as a value is not very helpful for users. And it's a bit troublesome to specifically set up a placeholder.

Testing

I have run the help_test.go , but it can't pass . Because I consider if we should add a switch, otherwise it will affect the compatibility of the previous version

Release Notes

Change flag show placeholder to show flag initial type.

@jokemanfire jokemanfire requested a review from a team as a code owner January 9, 2025 09:20
@jokemanfire
Copy link
Contributor Author

@Skeeve Thank you, could you please take a look

@Skeeve
Copy link
Contributor

Skeeve commented Jan 9, 2025

@Skeeve Thank you, could you please take a look

I can, but I cannot approve.

@avorima
Copy link
Contributor

avorima commented Jan 9, 2025

It's currently possible to get the following help output with the Usage field:

Usage: "string"
Output:
   --bar value  string
Usage: "`string`"
Output:
   --bar string  string

I think either one of those cases could be changed to overwrite the placeholder without setting a usage.

flag.go Outdated Show resolved Hide resolved
@abitrolly
Copy link
Contributor

This whole section is questionable.

cli/flag.go

Lines 300 to 310 in 7fc43e7

// enforce DocGeneration interface on flags to avoid reflection
df, ok := f.(DocGenerationFlag)
if !ok {
return ""
}
placeholder, usage := unquoteUsage(df.GetUsage())
needsPlaceholder := df.TakesValue()
if needsPlaceholder && placeholder == "" {
placeholder = defaultPlaceholder
}

The unquoteUsage() function parses flag value type from the usage (!) string (#333). I am +1 on dropping this workaround if this PR works.

@avorima
Copy link
Contributor

avorima commented Jan 9, 2025

I didn't know this was a workaround to get stdlib flag functionality. Might I add two points to consider:

  • If the interface method were Type() string it would allow inter-op with pflag's Value interface.
  • reflect.Type.Name() works for strings, numbers and bools but returns the empty string for slices and maps. I think for those the default value should still apply.

@jokemanfire
Copy link
Contributor Author

jokemanfire commented Jan 10, 2025

Thanks all @avorima @abitrolly @Skeeve . I will make a summary based on your suggestions and explain my ideas ,

  1. I would like to keep that method https://github.com/urfave/cli/issues/333 , because somebody may want to custom flag type by themself .
  2. If reflect.Type.Name() is not enough for get all base type , I will try to use other method.

my next step will be:

  1. Improve this code to ensure that all basic types can be displayed, and ensure that custom priority is > the default flag type (like string., bool , etc )priority.
  2. Fix CI
  3. Fix doc

@jokemanfire
Copy link
Contributor Author

jokemanfire commented Jan 10, 2025

That's a nice idea , May I consider it . From the current perspective, using reflection should be sufficient.

@jokemanfire jokemanfire force-pushed the dev2 branch 4 times, most recently from e922a04 to 166beb6 Compare January 10, 2025 05:50
print flag type first.

Signed-off-by: jokemanfire <[email protected]>
@Skeeve
Copy link
Contributor

Skeeve commented Jan 10, 2025

0cb6639#diff-f09230a06a76ff761bbdcc9b788aa057cc5caa463f1bd68e67b14422938622d4R110

I think the "[]" is of no additional value and should be omitted.

  1. Brackets have a well known meaning in usage texts. They are confusing here
  2. Slice flags are no different from non-slice flags. There is no special parameter syntax for them

I think you should avoid the "[]" marker.

@jokemanfire
Copy link
Contributor Author

jokemanfire commented Jan 10, 2025

0cb6639#diff-f09230a06a76ff761bbdcc9b788aa057cc5caa463f1bd68e67b14422938622d4R110

I think the "[]" is of no additional value and should be omitted.

  1. Brackets have a well known meaning in usage texts. They are confusing here
  2. Slice flags are no different from non-slice flags. There is no special parameter syntax for them

I think you should avoid the "[]" marker.

That's for these type like UintSliceFlag's flag can print '[]uint64' and reflect.Type.Name() can't return the slice type . Or may I print like this 'uint64s' for different from 'uint64' type?

@Skeeve
Copy link
Contributor

Skeeve commented Jan 10, 2025

That's for these type like UintSliceFlag's flag can print '[]uint64' and reflect.Type.Name() can't return the slice type . Or may I print like this 'uint64s' for different from 'uint64' type?

Why would you want to distinguish a uint64 from a slice of uint64. It transfers no additional meaning to the reader. If a flag is a slice flag, the only difference in usage is, that you can use the flag several times. This is usually stated in the usage text.

@jokemanfire
Copy link
Contributor Author

That's for these type like UintSliceFlag's flag can print '[]uint64' and reflect.Type.Name() can't return the slice type . Or may I print like this 'uint64s' for different from 'uint64' type?

Why would you want to distinguish a uint64 from a slice of uint64. It transfers no additional meaning to the reader. If a flag is a slice flag, the only difference in usage is, that you can use the flag several times. This is usually stated in the usage text.

You're right. How about just print the slice type ?
example:

Flags: []cli.Flag{
			&cli.StringSliceFlag{
				Name:  "greeting",
				Usage: "Pass multiple greetings",
			},
		},

=> --greeting string [ --greeting string ] Pass multiple greetings

@abitrolly
Copy link
Contributor

Actually, the placeholder hack is documented https://cli.urfave.org/v3/examples/flags/#placeholder-values

Maybe it doesn't need to be overcomplicated. Maybe stub basic types and provide an extension to explicitly format placeholder name for other things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants