Skip to content

Print StringToInt and StringToInt64 defaults in sorted order - #515

Open
SulimanAbdulrazzaq wants to merge 1 commit into
spf13:masterfrom
SulimanAbdulrazzaq:fix/string-to-int-sorted-defaults
Open

SulimanAbdulrazzaq wants to merge 1 commit into
spf13:masterfrom
SulimanAbdulrazzaq:fix/string-to-int-sorted-defaults

Conversation

@SulimanAbdulrazzaq

Copy link
Copy Markdown

#365 made stringToStringValue.String() sort its keys, so a StringToString flag's (default [...]) text in FlagUsages() is stable between runs (#362). StringToInt and StringToInt64 never got the same change. Their String() methods still range over the map directly, so a flag with more than one default key prints them in Go's randomized map order:

f := pflag.NewFlagSet("test", pflag.ContinueOnError)
f.StringToInt("s2i", map[string]int{"c": 3, "a": 1, "d": 4, "b": 2}, "usage")
f.Lookup("s2i").Value.String() // e.g. "[a=1,d=4,b=2,c=3]", different from run to run

This is the problem #362 described: --help output, and any usage text or docs generated from it, changes on every run.

This PR sorts the keys in both String() methods, the same way string_to_string.go does. sort.Strings keeps it compatible with the Go 1.12 job in CI.

Tests

TestS2IUsageSorted and TestS2I64UsageSorted set up a four-key default and check, 100 times, that String() returns [a=1,b=2,c=3,d=4] and that FlagUsages() contains (default [a=1,b=2,c=3,d=4]).

  • Without the change, both fail on the first iteration, for example expected String() to be "[a=1,b=2,c=3,d=4]" but got: "[c=3,a=1,d=4,b=2]".
  • With it, they pass, and so does go test -race -v ./....
  • go vet ./... is clean and gofmt reports no changes.

spf13#365 sorted the keys in stringToStringValue.String() so that the
"(default [...])" text in FlagUsages no longer changes from run to run
(spf13#362). The stringToInt and stringToInt64 values still range over the map
directly, so a flag with more than one default key prints them in Go's
randomized map order and --help output, or docs generated from it,
differs between runs.

Sort the keys in both String() methods the same way.
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@SulimanAbdulrazzaq

Copy link
Copy Markdown
Author

@tomasaschan when you have a moment, could one of you approve the workflow runs and take a look at this one? No checks have run on the current head yet.

@tomasaschan

Copy link
Copy Markdown
Collaborator

The influx of PRs, especially automated ones, to this repo has been big enough that I have stopped interacting with any PRs that don't show the CLA is signed. Do that, and it gets on the review queue.

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