Skip to content
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

[CLEANUP] Replace experimental maps and slices with stdlib #2993

Merged

Conversation

Juneezee
Copy link
Contributor

@Juneezee Juneezee commented Nov 2, 2024

The experimental functions are now available in the standard library in Go 1.23 1.

The only difference is that maps.Keys and maps.Values in the standard library return an iterator, while maps.Keys and maps.Values in the golang.org/x/exp package return a slice.

Footnotes

  1. https://go.dev/doc/go1.23#new-unique-package

The experimental functions are now available in the standard library
in Go 1.23 [1].

[1]: https://go.dev/doc/go1.23#new-unique-package

Signed-off-by: Eng Zer Jun <[email protected]>
Copy link
Member

@AnomalRoil AnomalRoil left a comment

Choose a reason for hiding this comment

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

A few questions I guess

Comment on lines +99 to +101
return slices.SortedFunc(bes, func(a, b V) int {
return cmp.Compare(a.Priority(), b.Priority())
})
Copy link
Member

Choose a reason for hiding this comment

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

Why can't you use slices.Sorted directly? I'd expect Priority() to return a cmp.Ordered value, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't use slices.Sorted.

image

You can see the types permitted by cmp.Ordered here: https://pkg.go.dev/cmp#Ordered. We are using generics so it is not possible to use slices.Sorted here.

Comment on lines +345 to +346
_, ok := c.vars["core.sshCommand"]
assert.True(t, ok)
Copy link
Member

Choose a reason for hiding this comment

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

why not use SortedKeys here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old code

assert.Contains(t, maps.Keys(c.vars), "core.sshCommand")

was just testing whether the core.sshCommand key exists in the c.vars map or not.

Obviously we can do assert.Contains(t, set.SortedKeys(c.vars), "core.sshCommand") too to test whether core.sshCommand exists in c.vars or not. But _, ok := c.vars["core.sshCommand"] does the job in a much simplier way

Copy link
Member

@dominikschulz dominikschulz left a comment

Choose a reason for hiding this comment

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

Thanks!

@dominikschulz dominikschulz merged commit 7ac2990 into gopasspw:master Nov 6, 2024
8 checks passed
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