-
Notifications
You must be signed in to change notification settings - Fork 367
remove uses of errors.Is, which requires go1.13, move go1.16/go1.21 tests to separate file #448
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
Conversation
|
@tomasaschan PTAL |
|
Given the outrage at recent breaking changes being pushed without moving to v2, I don't think we can bump the required go version before that either. So whatever we have that requires later versions should either be rewritten to work on 1.12, or be moved into flags that can be feature toggled (like you did for the Func/BoolFunc stuff). I have been meaning to start a conversation about what the path to 2.0 looks like, and what type of changes we want to get in there, but I don't think the right medium for such a discussion is currently available (which is one of the reasons I filed for widening my permissions). |
|
Yeah, the breaking changes are slightly more problematic, but should be solved now with your changes merged (and follow up in #447) w.r.t. updating the minimum conversion; I think it's fairly reasonable to update to go1.13, which is still very conservative, but if its a big concern; it looks like the only blocker for keeping it at go1.12 is the Note that those changes already shipped; this PR is only adjusting the |
|
Yeah, I think the correct course of action is probably to revert the Then we can start building a list of things we'd want to do differently in 2.0 (and, perhaps, e.g. explicitly call out that we won't consider bumping required go version a breaking change starting from 2.0). I have a bunch of other things I'd want a more recent go version for too (generics!) but I've decided that all of that has to wait until we've figured out what the path to v2 looks like. |
52cae09 to
3b4aebd
Compare
|
OK 👍 I dropped the first commit, and replaced it with a commit that removes the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
I think both #447 and this one would be good to include in v1.0.10, because this one fixes a bug where the That said; I wouldn't consider either to be critical, or at least, using go1.12 would be a very niche-case (it happens, but usually it would be in some internal codebases in companies maintaining their own LTS Go versions - I know Google itself did so for a long time. I think it's fine to keep #449 open as draft; you can rebase it once this one's merged, but I can include it in my follow-up to address some linting issues. |
|
Perhaps easier; I cherry-picked your commit in this branch 😄 |
Commit 1bf832c introduced the use of `errors.Is`, which was added in [go1.13], but the `go.mod` was not updated to reflect this, causing compile failures on go1.12: docker run -it --rm -v ./:/pflag -w /pflag golang:1.12 sh -c 'go test -v ./...' # github.com/spf13/pflag [github.com/spf13/pflag.test] ./flag.go:1190:7: undefined: errors.Is ./flag.go:1219:7: undefined: errors.Is ./bool_func_test.go:86:28: cannot use stdFSet (type *flag.FlagSet) as type BoolFuncFlagSet in argument to runCase: *flag.FlagSet does not implement BoolFuncFlagSet (missing BoolFunc method) ... As the error that is tested will not be wrapped, we can omit the `errors.Is`, and instead continue doing a straight comparison. [go1.13]: https://pkg.go.dev/errors@go1.13#Is Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Commit 69bc3bd added support for Func() and BoolFunc() to match stdlib. However, the Func method was added in [go1.16.0], and BoolFunc in [go1.21.0], so running the tests on older versions of Go would fail; docker run -it --rm -v ./:/pflag -w /pflag golang:1.21 sh -c 'go test -v ./...' # github.com/spf13/pflag [github.com/spf13/pflag.test] ./bool_func_test.go:86:28: cannot use stdFSet (type *flag.FlagSet) as type BoolFuncFlagSet in argument to runCase: *flag.FlagSet does not implement BoolFuncFlagSet (missing BoolFunc method) ./bool_func_test.go:113:21: undefined: io.Discard ./bool_func_test.go:116:28: cannot use stdFSet (type *flag.FlagSet) as type BoolFuncFlagSet in argument to runCase: *flag.FlagSet does not implement BoolFuncFlagSet (missing BoolFunc method) ./bool_func_test.go:139:7: undefined: errors.Is ./func_test.go:92:28: cannot use stdFSet (type *flag.FlagSet) as type FuncFlagSet in argument to runCase: *flag.FlagSet does not implement FuncFlagSet (missing Func method) ./func_test.go:119:21: undefined: io.Discard ./func_test.go:122:28: cannot use stdFSet (type *flag.FlagSet) as type FuncFlagSet in argument to runCase: *flag.FlagSet does not implement FuncFlagSet (missing Func method) ./func_test.go:145:7: undefined: errors.Is ./func_test.go:145:7: too many errors FAIL github.com/spf13/pflag [build failed] This patch moves the tests to a separate file that is not built for older versions of Go. [go1.16.0]: https://pkg.go.dev/flag@go1.16.0#Func [go1.21.0]: https://pkg.go.dev/flag@go1.21.0#BoolFunc Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Since 1.12 is what we specify in go.mod, and therefore implicitly is what we promise to work with, we should ensure that we don't introduce changes which break this promise (e.g. as 1bf832c). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
7e80bd3 to
7e4dfb1
Compare
This PR contains the following updates: | Package | Change | Age | Confidence | |---|---|---|---| | [github.com/spf13/pflag](https://github.com/spf13/pflag) | `v1.0.9` -> `v1.0.10` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### 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> </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:eyJjcmVhdGVkSW5WZXIiOiI0MS45MS4yIiwidXBkYXRlZEluVmVyIjoiNDEuOTEuMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiS2luZC9DaG9yZSIsInJ1bi1lbmQtdG8tZW5kLXRlc3RzIl19--> <!--start release-notes-assistant--> <!--URL:https://code.forgejo.org/forgejo/runner--> - other - [PR](https://code.forgejo.org/forgejo/runner/pulls/932): <!--number 932 --><!--line 0 --><!--description VXBkYXRlIG1vZHVsZSBnaXRodWIuY29tL3NwZjEzL3BmbGFnIHRvIHYxLjAuMTA=-->Update module github.com/spf13/pflag to v1.0.10<!--description--> <!--end release-notes-assistant--> Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/932 Reviewed-by: earl-warren <earl-warren@noreply.code.forgejo.org> Co-authored-by: Renovate Bot <bot@kriese.eu> Co-committed-by: Renovate Bot <bot@kriese.eu>
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> --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MC4zNy4xIiwidXBkYXRlZEluVmVyIjoiNDEuMTczLjEiLCJ0YXJnZXRCcmFuY2giOiJtYXN0ZXIiLCJsYWJlbHMiOltdfQ==--> See merge request alpine/infra/build-server-status!21
relates to:
remove uses of errors.Is, which requires go1.13
Commit 1bf832c introduced the use of
errors.Is, which was added in go1.13, but thego.modwas notupdated to reflect this, causing compile failures on go1.12:
As the error that is tested will not be wrapped, we can omit the
errors.Is,and instead continue doing a straight comparison.
move Func, BoolFunc, tests as they require go1.21
Commit 69bc3bd added support for Func()
and BoolFunc() to match stdlib. However, the Func method was added in
go1.16.0, and BoolFunc in go1.21.0, so running the tests on older
versions of Go would fail;
This patch moves the tests to a separate file that is not built for older
versions of Go.