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

Run flag actions when Value is set #1973

Open
samlaf opened this issue Sep 29, 2024 · 11 comments
Open

Run flag actions when Value is set #1973

samlaf opened this issue Sep 29, 2024 · 11 comments
Labels
area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this status/waiting-for-response Waiting for response from original requester

Comments

@samlaf
Copy link

samlaf commented Sep 29, 2024

Believe that Actions are only run when a flag is set. This means that when I have an Action which I want to run even on the default Value, I have to set HasBeenSet to true, like below

var MaxBlobLengthBytes = 0
[...] // pseudo code for flag definition below
		cli.StringFlag{
			Name:    "foo",
			Value:   "16MiB",
			HasBeenSet: true, // set this to true to force action to run
			Action: func(_ *cli.Context, maxBlobLengthStr string) error {
				// parse the string to a uint64 and set the maxBlobLengthBytes var to be used by ReadConfig()
				numBytes, err := utils.ParseBytesAmount(maxBlobLengthStr)
				if err != nil {
					return fmt.Errorf("failed to parse max blob length flag: %w", err)
				}
				if numBytes == 0 {
					return fmt.Errorf("max blob length is 0")
				}
				if numBytes > MaxAllowedBlobSize {
					return fmt.Errorf("excluding disperser constraints on max blob size, SRS points constrain the maxBlobLength configuration parameter to be less than than %d bytes", MaxAllowedBlobSize)
				}
				MaxBlobLengthBytes = numBytes
				return nil
			},
		},

Is this intended behavior? Should we not also run an action when a Value is set?

@samlaf samlaf added area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this labels Sep 29, 2024
@dearchap
Copy link
Contributor

@samlaf That is the intended behaviour. You cannot trigger a flag action on default. Also the hasBeenSet is a private field in StringFlag so you cannot really set it.

@samlaf
Copy link
Author

samlaf commented Sep 29, 2024

@dearchap hmm that's weird, because it's public on my import and I've been using it with this behavior (tested that it's working):
image

Could you explain or link me to a place where the intended behaviour is explained? My use case seems like a valid behaviour to me.

@dearchap
Copy link
Contributor

@samlaf you are actually using v2. The v3 tag on this issue confused me. Here is the code which decides whether to call a flag action or not. https://github.com/urfave/cli/blob/v2-maint/app.go#L473 . Also note that v2 is in maint mode so any changes in this behavior need to be done on v3.

@samlaf
Copy link
Author

samlaf commented Sep 30, 2024

@dearchap oh yes my bad… opened it initially woth the v3 template but then realized I was using v2 so erased the template msg but left the tag.

Is v3 production ready right now? And if I switch to it how would I reproduce the current behavior I’m using of running the action on the default value (nice to have default be something human readable as it’s shown as an example to users in the —help output)

@dearchap
Copy link
Contributor

v3 is in beta state and is mostly usable.

@dearchap dearchap added the status/waiting-for-response Waiting for response from original requester label Oct 19, 2024
@dearchap
Copy link
Contributor

@samlaf Would you like to make a PR for this ?

@samlaf
Copy link
Author

samlaf commented Oct 26, 2024

Don’t have time these days so can’t promise anything. Unless you point me to the exact/approximate where the change would be. Not familiar with Urfave internal codebase, let alone v3.

@dearchap
Copy link
Contributor

You can look at command.Run which calls runFlagActions

@dearchap
Copy link
Contributor

@samlaf It doesnt really make sense to run a Flag Action all the time. It defeats the purpose. It would just be a block of code that you could put in the Command.Action . I'm going to close this issue .

@dearchap dearchap closed this as not planned Won't fix, can't repro, duplicate, stale Oct 31, 2024
@samlaf
Copy link
Author

samlaf commented Oct 31, 2024

@dearchap its about locality of code. If I have 3 such flags then command.Action becomes crowded no? Makes more sense for me to have the parsing right next to the flag.

@dearchap dearchap reopened this Oct 31, 2024
@dearchap
Copy link
Contributor

I agree but this is a very atypical use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v3 relates to / is being considered for v3 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

2 participants