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

App.ToMan() and App.ToMarkdown() don't respect DefaultText #1689

Closed
3 tasks done
alexg-axis opened this issue Mar 2, 2023 · 13 comments
Closed
3 tasks done

App.ToMan() and App.ToMarkdown() don't respect DefaultText #1689

alexg-axis opened this issue Mar 2, 2023 · 13 comments
Labels
area/v2 relates to / is being considered for v2 kind/bug describes or fixes a bug status/triage maintainers still need to look into this status/waiting-for-response Waiting for response from original requester
Milestone

Comments

@alexg-axis
Copy link

My urfave/cli version is

v2.24.4

Checklist

  • Are you running the latest v2 release? The list of releases is here.
  • Did you check the manual for your release? The v2 manual is here
  • Did you perform a search about this problem? Here's the GitHub guide about searching.

Dependency Management

  • My project is using go modules.

Describe the bug

When using App.ToMan() or App.ToMarkdown(), the generated text uses the Value of a flag no matter if DefaultText is specified or not. This makes it inconsistent to the output of the help command.

To reproduce

Include a flag like the following in your app:

&cli.StringFlag{
  Name:        "foo",
  Value:       "default value"
  DefaultText: "default text"
}

Observed behavior

./main help
--foo (default: default text)
x, _ := App.ToMan()
fmt.Println(x)
...
\fB--foo\fP="": (default: default value)
...
x, _ := App.ToMarkdown()
fmt.Println(x)
...
**--foo**="": (default: default value)
....

Expected behavior

...
\fB--foo\fP="": (default: default text)
...
**--foo**="": (default: default text)

Additional context

None.

Want to fix this yourself?

If confirmed, I'd be happy to fix it.

Run go version and paste its output here

go version go1.19.6 darwin/amd64

Run go env and paste its output here

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/path/to/Library/Caches/go-build"
GOENV="/path/to/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/path/to/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/path/to/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.19.6/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.19.6/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.19.6"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/path/to/example/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/path/to/go-build3156486010=/tmp/go-build -gno-record-gcc-switches -fno-common"
@alexg-axis alexg-axis added area/v2 relates to / is being considered for v2 kind/bug describes or fixes a bug status/triage maintainers still need to look into this labels Mar 2, 2023
@alexg-axis alexg-axis changed the title App.ToMan() and App.ToMarkdown() don't respect DefaultTest App.ToMan() and App.ToMarkdown() don't respect DefaultText Mar 2, 2023
@skelouse
Copy link
Contributor

skelouse commented Mar 5, 2023

This is a bigger issue than I anticipated. I tried the following which gets the expected output but reopens an issue, and as expected breaks markdown and man tests.

Essentially reverting: #1206
Which would re-open this issue.

// GetDefaultText returns the default text for this flag
func (f *StringFlag) GetDefaultText() string {
	if f.DefaultText != "" {
		return f.DefaultText
	}
	
	// Changed from
	// if f.defaultValue == "" {
	// 	return f.defaultValue
	// }
	// return fmt.Sprintf("%q", f.defaultValue)
	return f.String()
}

And

// flagDetails returns a string containing the flags metadata
func flagDetails(flag DocGenerationFlag) string {
	description := flag.GetUsage()
	
	// Changed from
	// value := flag.GetValue()
	value := flag.GetDefaultText()
	
	if value != "" {
		description += " (default: " + value + ")"
	}
	return ": " + description
}

@dearchap
Copy link
Contributor

dearchap commented Mar 6, 2023

@skelouse Yes. The help test/default text/values etc have been fine tuned over a long period to get the desired results. Its very tough to make changes without breaking something else. Do you want to work on it or else I can take a look

@skelouse
Copy link
Contributor

skelouse commented Mar 6, 2023

@skelouse Yes. The help test/default text/values etc have been fine tuned over a long period to get the desired results. Its very tough to make changes without breaking something else. Do you want to work on it or else I can take a look

Thanks for the offer, I'd rather not pick and pry at these!

@alexg-axis
Copy link
Author

@dearchap have you had the time to investigate this issue?

@dearchap
Copy link
Contributor

@alexg-axis Not yet. Does #1721 solve your problem ?

@alexg-axis
Copy link
Author

No, unfortunately not. It makes use of the default value and not default text (if set).

@dearchap
Copy link
Contributor

@alexg-axis Try this PR branch fix

@alexg-axis
Copy link
Author

@dearchap Thanks, I think we're getting close. This did however also change the behavior of things such as IntFlag with a set Value. Previously it would be documented as (default: 25), for example, when it now always becomes (default: 0).

If it's a "required side effect" of the fix, then I don't have too much against having to specify both Value and DefaultText, but it would of course be nicer to only have DefaultText override values so that I don't have to specify the default twice.

@dearchap
Copy link
Contributor

@alexg-axis Here is the order of priority in display default

  • DefaultText
  • Value that was set initially in the field initializer
  • Current Value set in the field.

So which condition are you hitting ?

@alexg-axis
Copy link
Author

alexg-axis commented Apr 18, 2023

Flag definition:

&cli.IntFlag{
  Name:  "some-flag",
  Usage: "Usage";
  Value: 25,
}

Previous output:

**--some-flag**="": Usage (default: 25)
\fB--some-flag\fP="": Usage (default: 25)

New output:

**--some-flag**="": Usage (default: 0)
\fB--some-flag\fP="": Usage (default: 0)

Here I would assume that I'm hitting the second item in your list - an initial value set in the field initializer. Thus I would expect the new output to match the old output here.

@meatballhat meatballhat added this to the Release 2.x milestone Jun 21, 2023
@tetra12
Copy link

tetra12 commented Sep 23, 2023

Still not working v2.25.7:

Flags: []cli.Flag{
  &cli.StringFlag{
  Name:        "name",
  Aliases:     []string{"n"},
  Usage:       "Route name",
  Required:    true,
  Value:       "foo",
  DefaultText: "foo",
  Action:      s.Delete,
  },
}

NAME:
route delete - delete

USAGE:
route delete [command options] [arguments...]

OPTIONS:
--name value, -n value Route name (default: foo)
--help, -h show help
xxxx 18:47:44 Required flag "name" not set

@dearchap
Copy link
Contributor

@alexg-axis Please try the PR fix.

@dearchap dearchap added the status/waiting-for-response Waiting for response from original requester label Nov 29, 2023
@alexg-axis
Copy link
Author

@dearchap seems to be working well! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v2 relates to / is being considered for v2 kind/bug describes or fixes a bug status/triage maintainers still need to look into this status/waiting-for-response Waiting for response from original requester
Projects
None yet
Development

No branches or pull requests

5 participants