Skip to content

Conversation

@MidnightRocket
Copy link

@MidnightRocket MidnightRocket commented Mar 18, 2025

The defaultIsZeroValue function does not check on the DefValue field, for generic Value types., but instead wrongly checks on the flag Value field.
This results in the help screen wrongly displays the default value for a custom Value option, even though the default value is a "zerovalue".

Reproduce

hellocmd --my-option "some-value" -h

Should not display the default value in the help screen, but does anyways without this patch.

Example code to reproduce

package main
import (
	"fmt"
	"github.com/spf13/cobra"
	"os"
)

var myOption StringOption

func init() {
	Command.Flags().Var(&myOption, "my-option", "My custom option")
}

func main() {
	if err := Command.Execute(); err != nil {
		fmt.Println(err)
		os.Exit(1)
	}
}

var Command = &cobra.Command{
	Use:   "hellocmd",
	Short: "Hello ",
	Long:  "Hello long",
	Run: func(cmd *cobra.Command, args []string) {
	},
}

type StringOption struct {
	value string
}

func (s *StringOption) Type() string {
	return "string"
}

func (s *StringOption) String() string {
	return s.value
}

func (s *StringOption) Set(s2 string) error {
	s.value = s2
	return nil
}

Similar / related PR

#361

Is related, however solves this problem specifically for isBoolFlag()=true custom Value types.
While this additionally solves for other custom Value types.

@CLAassistant
Copy link

CLAassistant commented Mar 18, 2025

CLA assistant check
All committers have signed the CLA.

@MidnightRocket MidnightRocket force-pushed the fix-default-is-zerovalue-for-generic-value-type branch from c6f8d76 to 9edfc8d Compare March 18, 2025 14:17
@tomasaschan
Copy link
Collaborator

Change LGTM, but it'd be nice to have it covered by tests.

@MidnightRocket
Copy link
Author

I completely agree. However I am quite new to go, and have no experience with writing tests in go 😅.
I am however willing to give it a try, and learn along the way.
I would appreciate some pointers if you have any to give 😃.

I presume that I should put the test in the existing flag_test.go file?
However this test would require some boilerplate such as a custom implementation of Value type, should this also be in the same file?

@tomasaschan
Copy link
Collaborator

Yes, tests for this go in flag_test.go! Here are the official getting-started docs for testing in Go in case you need a reference.

I imagine you should be able to extend this test to ensure your change actually fixes the issue; in other words, with a change to that test but without the feature change in this PR, the test should fail - then applying this PR change again should make the test green again.

MidnightRocket added a commit to MidnightRocket/pflag that referenced this pull request Mar 21, 2025
@MidnightRocket
Copy link
Author

I have extended the TestPrintDefault to also check for this edge case.
I also took the liberty to improve the readability of the error output of the test, as I found it quite difficult to read. This however can be reverted, if needed 😃.

Copy link
Collaborator

@tomasaschan tomasaschan left a comment

Choose a reason for hiding this comment

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

Sorry this took a while - been swamped with other things... I have one small nit, but given the delayed response here I'm not going to block on that.

/lgtm

@MidnightRocket MidnightRocket force-pushed the fix-default-is-zerovalue-for-generic-value-type branch from 260afbc to c963093 Compare April 1, 2025 19:24
@tomasaschan
Copy link
Collaborator

/lgtm

@tomasaschan
Copy link
Collaborator

Thank you for your contribution!

@tomasaschan tomasaschan merged commit 19c9c40 into spf13:master Apr 1, 2025
5 checks passed
@MidnightRocket MidnightRocket deleted the fix-default-is-zerovalue-for-generic-value-type branch April 1, 2025 20:50
project-mirrors-bot-tu bot pushed a commit to project-mirrors/forgejo-runner-as-gitea-act-runner-fork that referenced this pull request Aug 2, 2025
This PR contains the following updates:

| Package | Change | Age | Confidence |
|---|---|---|---|
| [github.com/spf13/pflag](https://github.com/spf13/pflag) | `v1.0.6` -> `v1.0.7` | [![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fspf13%2fpflag/v1.0.7?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fspf13%2fpflag/v1.0.6/v1.0.7?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>spf13/pflag (github.com/spf13/pflag)</summary>

### [`v1.0.7`](https://github.com/spf13/pflag/releases/tag/v1.0.7)

[Compare Source](spf13/pflag@v1.0.6...v1.0.7)

#### What's Changed

- Fix defaultIsZeroValue check for generic Value types by [@&#8203;MidnightRocket](https://github.com/MidnightRocket) in spf13/pflag#422
- feat: Use structs for errors returned by pflag. by [@&#8203;eth-p](https://github.com/eth-p) in spf13/pflag#425
- Fix typos by [@&#8203;co63oc](https://github.com/co63oc) in spf13/pflag#428
- fix [#&#8203;423](spf13/pflag#423) : Add helper function and some documentation to parse shorthand go test flags. by [@&#8203;valdar](https://github.com/valdar) in spf13/pflag#424
- add support equivalent to golang flag.TextVar(), also fixes the test failure as described in [#&#8203;368](spf13/pflag#368) by [@&#8203;hujun-open](https://github.com/hujun-open) in spf13/pflag#418
- add support for Func() and BoolFunc() [#&#8203;426](spf13/pflag#426) by [@&#8203;LeGEC](https://github.com/LeGEC) in spf13/pflag#429
- fix: correct argument length check in FlagSet.Parse by [@&#8203;ShawnJeffersonWang](https://github.com/ShawnJeffersonWang) in spf13/pflag#409
- fix usage message for func flags, fix arguments order by [@&#8203;LeGEC](https://github.com/LeGEC) in spf13/pflag#431
- Add support for time.Time flags by [@&#8203;max-frank](https://github.com/max-frank) in spf13/pflag#348

#### New Contributors

- [@&#8203;MidnightRocket](https://github.com/MidnightRocket) made their first contribution in spf13/pflag#422
- [@&#8203;eth-p](https://github.com/eth-p) made their first contribution in spf13/pflag#425
- [@&#8203;co63oc](https://github.com/co63oc) made their first contribution in spf13/pflag#428
- [@&#8203;valdar](https://github.com/valdar) made their first contribution in spf13/pflag#424
- [@&#8203;hujun-open](https://github.com/hujun-open) made their first contribution in spf13/pflag#418
- [@&#8203;LeGEC](https://github.com/LeGEC) made their first contribution in spf13/pflag#429
- [@&#8203;ShawnJeffersonWang](https://github.com/ShawnJeffersonWang) made their first contribution in spf13/pflag#409
- [@&#8203;max-frank](https://github.com/max-frank) made their first contribution in spf13/pflag#348

**Full Changelog**: spf13/pflag@v1.0.6...v1.0.7

</details>

---

### Configuration

📅 **Schedule**: Branch creation - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC), Automerge - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS40My41IiwidXBkYXRlZEluVmVyIjoiNDEuNDMuNSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/794
Reviewed-by: earl-warren <[email protected]>
Co-authored-by: Renovate Bot <[email protected]>
Co-committed-by: Renovate Bot <[email protected]>
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.

3 participants