-
Notifications
You must be signed in to change notification settings - Fork 7
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
Sort keys returned by AllKeys in reverse order #34
Conversation
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.
Left a nitty comment, but this should be OK.
@@ -1801,9 +1802,20 @@ func (v *Viper) AllKeys() []string { | |||
for x := range m { | |||
a = append(a, x) | |||
} | |||
return a | |||
sort.Strings(a) |
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.
Maybe a reverse order insert sort in the for loop above might make more sense, but since I don't presume this doesn't get called much, this should be fine.
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.
👍 suggestion for using the sort.StringSlice + Reverse interface but lgtm
@@ -1801,9 +1802,20 @@ func (v *Viper) AllKeys() []string { | |||
for x := range m { | |||
a = append(a, x) | |||
} | |||
return a | |||
sort.Strings(a) | |||
return ReverseStringSlice(a) |
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.
could we use sort.Reverse for this?
sort.Sort(sort.Reverse(sort.StringSlice(a)))
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.
No, unfortunately this repo requires 1.14, generics aren't available.
Abandoning this PR, in favor of this approach: #36 |
Returning keys in a stable manner prevents non-determinism. A bug happening in
allSettings
manifests under certain orders of keys, by using reverse order this bug is never triggered.