add support equivalent to golang flag.TextVar(), also fixes the test failure as described in #368 #418

hujun-open · 2024-12-27T20:08:34Z

No description provided.

…failure as described in spf13#368

CLAassistant · 2024-12-27T20:08:39Z

All committers have signed the CLA.

hujun-open · 2025-05-08T15:49:35Z

Could this PR be looked? it has been a while since opened...

tomasaschan · 2025-05-09T07:28:46Z

text_test.go

+		if err != nil && tc.success == true {
+			t.Errorf("expected success, got %q", err)
+			continue
+		} else if err == nil && tc.success == false {
+			t.Errorf("expected failure, but succeeded")
+			continue
+		} else if tc.success {
+			parsedT := new(time.Time)
+			err := f.GetText("time", parsedT)
+			if err != nil {
+				t.Errorf("Got error trying to fetch the time flag: %v", err)
+			}
+			if !parsedT.Equal(tc.expected) {
+				t.Errorf("expected %q, got %q", tc.expected, parsedT)
+			}
+		}


I'm surprised the linter doesn't complain about the unnecessary nesting here. Here's a slight simplification (that also makes it clearer why we don't handle the !tc.success case on its own):

Suggested change

if err != nil && tc.success == true {

t.Errorf("expected success, got %q", err)

continue

} else if err == nil && tc.success == false {

t.Errorf("expected failure, but succeeded")

continue

} else if tc.success {

parsedT := new(time.Time)

err := f.GetText("time", parsedT)

if err != nil {

t.Errorf("Got error trying to fetch the time flag: %v", err)

}

if !parsedT.Equal(tc.expected) {

t.Errorf("expected %q, got %q", tc.expected, parsedT)

}

}

if err != nil {

// parsing failed...

if tc.success {

// ...and we did not expect it to

t.Errorf("Expected parsing to succeed, but got %v", err)

}

continue

}

if err == nil && !tc.success {

t.Errorf("Expected parsing failure, but parsing succeeded")

continue

}

parsedT := new(time.Time)

err := f.GetText("time", parsedT)

if err != nil {

t.Errorf("Got error trying to fetch the time flag: %v", err)

}

if !parsedT.Equal(tc.expected) {

t.Errorf("Expected %q, got %q", tc.expected, parsedT)

}

tomasaschan · 2025-05-09T07:31:59Z

text_test.go

+		expected time.Time
+	}{
+		{"2003-01-02T15:04:05Z", true, time.Date(2003, 1, 2, 15, 04, 05, 0, time.UTC)},
+		{"2003-01-02 15:05:01", false, time.Date(2002, 1, 2, 15, 05, 05, 07, time.UTC)},


This test case is confusing me: it's providing a string which I assume will fail parsing (?) but then it also provides a time.Date instance that doesn't match. That makes it hard to see, at a glance, why we expect this test to fail.

Can we maybe provide an empty time.Date instance to signal that we don't actually expect to look at it in the test?

Suggested change

{"2003-01-02 15:05:01", false, time.Date(2002, 1, 2, 15, 05, 05, 07, time.UTC)},

{"2003-01-02 15:05:01", false, time.Time{}}, // invalid date string

- return is missing in one error path of GetText(), fixed

hujun-open · 2025-05-10T23:30:09Z

Ok, I have updated based on your comments

btw: the nesting if was result of copy&paste from existing _test file, similar nesting if exists in bytes_test.go, count_test.go, ip_test.go and ipnet_test.go

tomasaschan

Nice, thank you for your contribution!

LeGEC · 2025-05-30T11:17:21Z

This PR also fixes #355

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 [@MidnightRocket](https://github.com/MidnightRocket) in spf13/pflag#422 - feat: Use structs for errors returned by pflag. by [@eth-p](https://github.com/eth-p) in spf13/pflag#425 - Fix typos by [@co63oc](https://github.com/co63oc) in spf13/pflag#428 - fix [#423](spf13/pflag#423) : Add helper function and some documentation to parse shorthand go test flags. by [@valdar](https://github.com/valdar) in spf13/pflag#424 - add support equivalent to golang flag.TextVar(), also fixes the test failure as described in [#368](spf13/pflag#368) by [@hujun-open](https://github.com/hujun-open) in spf13/pflag#418 - add support for Func() and BoolFunc() [#426](spf13/pflag#426) by [@LeGEC](https://github.com/LeGEC) in spf13/pflag#429 - fix: correct argument length check in FlagSet.Parse by [@ShawnJeffersonWang](https://github.com/ShawnJeffersonWang) in spf13/pflag#409 - fix usage message for func flags, fix arguments order by [@LeGEC](https://github.com/LeGEC) in spf13/pflag#431 - Add support for time.Time flags by [@max-frank](https://github.com/max-frank) in spf13/pflag#348 #### New Contributors - [@MidnightRocket](https://github.com/MidnightRocket) made their first contribution in spf13/pflag#422 - [@eth-p](https://github.com/eth-p) made their first contribution in spf13/pflag#425 - [@co63oc](https://github.com/co63oc) made their first contribution in spf13/pflag#428 - [@valdar](https://github.com/valdar) made their first contribution in spf13/pflag#424 - [@hujun-open](https://github.com/hujun-open) made their first contribution in spf13/pflag#418 - [@LeGEC](https://github.com/LeGEC) made their first contribution in spf13/pflag#429 - [@ShawnJeffersonWang](https://github.com/ShawnJeffersonWang) made their first contribution in spf13/pflag#409 - [@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. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  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]>

This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/spf13/pflag](https://github.com/spf13/pflag) | require | patch | `v1.0.6` -> `v1.0.10` | --- ### Release Notes <details> <summary>spf13/pflag (github.com/spf13/pflag)</summary> ### [`v1.0.10`](https://github.com/spf13/pflag/releases/tag/v1.0.10) [Compare Source](spf13/pflag@v1.0.9...v1.0.10) #### What's Changed - fix deprecation comment for (FlagSet.)ParseErrorsWhitelist by [@thaJeztah](https://github.com/thaJeztah) in [#447](spf13/pflag#447) - remove uses of errors.Is, which requires go1.13, move go1.16/go1.21 tests to separate file by [@thaJeztah](https://github.com/thaJeztah) in [#448](spf13/pflag#448) #### New Contributors - [@thaJeztah](https://github.com/thaJeztah) made their first contribution in [#447](spf13/pflag#447) **Full Changelog**: <spf13/pflag@v1.0.9...v1.0.10> ### [`v1.0.9`](https://github.com/spf13/pflag/releases/tag/v1.0.9) [Compare Source](spf13/pflag@v1.0.8...v1.0.9) #### What's Changed - fix: Restore ParseErrorsWhitelist name for now by [@tomasaschan](https://github.com/tomasaschan) in [#446](spf13/pflag#446) **Full Changelog**: <spf13/pflag@v1.0.8...v1.0.9> ### [`v1.0.8`](https://github.com/spf13/pflag/releases/tag/v1.0.8) [Compare Source](spf13/pflag@v1.0.7...v1.0.8) #### ⚠️ Breaking Change This version, while only a patch bump, includes a (very minor) breaking change: the `flag.ParseErrorsWhitelist` struct and corresponding `FlagSet.parseErrorsWhitelist` field have been renamed to `ParseErrorsAllowlist`. This should result in compilation errors in any code that uses these fields, which can be fixed by adjusting the names at call sites. There is no change in semantics or behavior of the struct or field referred to by these names. If your code compiles without errors after bumping to/past v1.0.8, you are not affected by this change. The breaking change was reverted in v1.0.9, by means of re-introducing the old names with deprecation warnings. The plan is still to remove them in a future release, so if your code does depend on the old names, please change them to use the new names at your earliest convenience. #### What's Changed - Remove Redundant "Unknown-Flag" Error by [@vaguecoder](https://github.com/vaguecoder) in [#364](spf13/pflag#364) - Switching from whitelist to Allowlist terminology by [@dubrie](https://github.com/dubrie) in [#261](spf13/pflag#261) - Omit zero time.Time default from usage line by [@mologie](https://github.com/mologie) in [#438](spf13/pflag#438) - implement CopyToGoFlagSet by [@pohly](https://github.com/pohly) in [#330](spf13/pflag#330) - flag: Emulate stdlib behavior and do not print ErrHelp by [@tmc](https://github.com/tmc) in [#407](spf13/pflag#407) - Print Default Values of String-to-String in Sorted Order by [@vaguecoder](https://github.com/vaguecoder) in [#365](spf13/pflag#365) - fix: Don't print ErrHelp in ParseAll by [@tomasaschan](https://github.com/tomasaschan) in [#443](spf13/pflag#443) - Reset args on re-parse even if empty by [@tomasaschan](https://github.com/tomasaschan) in [#444](spf13/pflag#444) #### New Contributors - [@vaguecoder](https://github.com/vaguecoder) made their first contribution in [#364](spf13/pflag#364) - [@dubrie](https://github.com/dubrie) made their first contribution in [#261](spf13/pflag#261) - [@mologie](https://github.com/mologie) made their first contribution in [#438](spf13/pflag#438) - [@pohly](https://github.com/pohly) made their first contribution in [#330](spf13/pflag#330) - [@tmc](https://github.com/tmc) made their first contribution in [#407](spf13/pflag#407) - [@tomasaschan](https://github.com/tomasaschan) made their first contribution in [#443](spf13/pflag#443) **Full Changelog**: <spf13/pflag@v1.0.7...v1.0.8> ### [`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 [@MidnightRocket](https://github.com/MidnightRocket) in [#422](spf13/pflag#422) - feat: Use structs for errors returned by pflag. by [@eth-p](https://github.com/eth-p) in [#425](spf13/pflag#425) - Fix typos by [@co63oc](https://github.com/co63oc) in [#428](spf13/pflag#428) - fix [#423](spf13/pflag#423) : Add helper function and some documentation to parse shorthand go test flags. by [@valdar](https://github.com/valdar) in [#424](spf13/pflag#424) - add support equivalent to golang flag.TextVar(), also fixes the test failure as described in [#368](spf13/pflag#368) by [@hujun-open](https://github.com/hujun-open) in [#418](spf13/pflag#418) - add support for Func() and BoolFunc() [#426](spf13/pflag#426) by [@LeGEC](https://github.com/LeGEC) in [#429](spf13/pflag#429) - fix: correct argument length check in FlagSet.Parse by [@ShawnJeffersonWang](https://github.com/ShawnJeffersonWang) in [#409](spf13/pflag#409) - fix usage message for func flags, fix arguments order by [@LeGEC](https://github.com/LeGEC) in [#431](spf13/pflag#431) - Add support for time.Time flags by [@max-frank](https://github.com/max-frank) in [#348](spf13/pflag#348) #### New Contributors - [@MidnightRocket](https://github.com/MidnightRocket) made their first contribution in [#422](spf13/pflag#422) - [@eth-p](https://github.com/eth-p) made their first contribution in [#425](spf13/pflag#425) - [@co63oc](https://github.com/co63oc) made their first contribution in [#428](spf13/pflag#428) - [@valdar](https://github.com/valdar) made their first contribution in [#424](spf13/pflag#424) - [@hujun-open](https://github.com/hujun-open) made their first contribution in [#418](spf13/pflag#418) - [@LeGEC](https://github.com/LeGEC) made their first contribution in [#429](spf13/pflag#429) - [@ShawnJeffersonWang](https://github.com/ShawnJeffersonWang) made their first contribution in [#409](spf13/pflag#409) - [@max-frank](https://github.com/max-frank) made their first contribution in [#348](spf13/pflag#348) **Full Changelog**: <spf13/pflag@v1.0.6...v1.0.7> </details> --- - [ ] If you want to rebase/retry this MR, check this box ---    See merge request alpine/infra/build-server-status!21

add support equivalent to golang flag.TextVar(), also fixes the test …

1118d46

…failure as described in spf13#368

hujun-open added 2 commits March 8, 2025 14:28

Merge remote-tracking branch 'upstream/master'

aae8e19

Merge remote-tracking branch 'upstream/master'

2a2b77a

tomasaschan reviewed May 9, 2025

View reviewed changes

- update text_test.go based on PR review comments

011db0c

- return is missing in one error path of GetText(), fixed

tomasaschan approved these changes May 14, 2025

View reviewed changes

tomasaschan merged commit 196624c into spf13:master May 15, 2025
5 checks passed

tomasaschan mentioned this pull request Jun 1, 2025

Generic Value and SliceValue implementation #359

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

add support equivalent to golang flag.TextVar(), also fixes the test failure as described in #368 #418

add support equivalent to golang flag.TextVar(), also fixes the test failure as described in #368 #418

Uh oh!

hujun-open commented Dec 27, 2024

Uh oh!

CLAassistant commented Dec 27, 2024 •

edited

Loading

Uh oh!

hujun-open commented May 8, 2025

Uh oh!

tomasaschan May 9, 2025

Uh oh!

tomasaschan May 9, 2025

Uh oh!

hujun-open commented May 10, 2025

Uh oh!

tomasaschan left a comment

Uh oh!

Uh oh!

LeGEC commented May 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	{"2003-01-02 15:05:01", false, time.Date(2002, 1, 2, 15, 05, 05, 07, time.UTC)},
	{"2003-01-02 15:05:01", false, time.Time{}}, // invalid date string

add support equivalent to golang flag.TextVar(), also fixes the test failure as described in #368 #418

add support equivalent to golang flag.TextVar(), also fixes the test failure as described in #368 #418

Uh oh!

Conversation

hujun-open commented Dec 27, 2024

Uh oh!

CLAassistant commented Dec 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hujun-open commented May 8, 2025

Uh oh!

tomasaschan May 9, 2025

Choose a reason for hiding this comment

Uh oh!

tomasaschan May 9, 2025

Choose a reason for hiding this comment

Uh oh!

hujun-open commented May 10, 2025

Uh oh!

tomasaschan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LeGEC commented May 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CLAassistant commented Dec 27, 2024 •

edited

Loading