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

Upgrade go-functional to v2 #3392

Merged
merged 7 commits into from
Aug 15, 2024

Conversation

BooleanCat
Copy link
Contributor

Is there a related GitHub Issue?

N/A.

What is this change about?

You're currently using version 1 of go-functional which I'm no longer maintaining. This upgrades go-functional to v2 and uses Go 1.23's native iter.Seq functionality for iterators - making all iterators in this library first-class iterators supported by the language.

There's two main packages of interest:

  1. it contains the iterators you are already using
  2. itx contains helpers to support iterator chaining

Unfortunately it's not possible to chain Map because of a limitation with Go's type system (you can't declare new generic arguments on a method not present on its type). But this was also a limitation of version 1 of go functional too.

Native Go iterators can be "lifted" to their chainable type by using itx.From(iterator).

If this eventually gets merged once you use Go 1.23 I'd be happy to comb through the code and find places where go-functional simplifies the codebase.

Does this PR introduce a breaking change?

Yes - you'll need to use Go 1.23. I notice you're using rc2 in your container images but 1.23 is not declared as the minimum version in the go.mod yet.

Acceptance Steps

No functional change.

Tag your pair, your PM, and/or team

N/A.

@BooleanCat
Copy link
Contributor Author

I'd also consider adding itx.FromSlice to facilitate what I think would be a common pattern of itx.From(slices.Values(slice)) -> itx.FromSlice(slice)

@BooleanCat
Copy link
Contributor Author

Note that merging this would break #3383 - which is why I wanted to make you aware of it now.

@danail-branekov
Copy link
Member

@BooleanCat

Thanks, Tom! We wanted to go straight to go-functional v2 but we decided not to as golang 1.23 is not ofcially released yet.
However, dependabot has bumped our containers already so I decided to give it a try. Unfortunately, the staticcheck linter panics with out code, hence we are not ready to do the switch on the code.
Once staticcheck is happy with go 1.23, we would have another round to adapt the other PR

go-functional is quite a cool library, it has been on my radar for a while and finally we got to it. I was going to give you a heads up that you have a new user but obviously you found that out yourself :)

@danail-branekov
Copy link
Member

FWIW, I have reported the issue with staticcheck: dominikh/go-tools#1578

@BooleanCat
Copy link
Contributor Author

@danail-branekov I've updated the PR to include the recent usages of go-functional.

In addition, I noticed use the the golang.org/x/exp/maps that is no longer necessary so I added an extra commit to stop using an experimental package.

@BooleanCat BooleanCat force-pushed the go-functional/v2 branch 2 times, most recently from 6e3b9e1 to ab01a80 Compare July 20, 2024 12:39
@danail-branekov
Copy link
Member

Thanks, Tom, this is awesome - this would save us quite some time once we want to go for go-functional v2

We are currently unable to merge that though until we bump to golang 1.23

@BooleanCat
Copy link
Contributor Author

BooleanCat commented Jul 27, 2024

Just FYI based on your feedback on another thread for collecting iterators that yield errors and values - in 2.0.0-beta.4+ there are two collect methods for iter.Seq2:

  1. TryCollect will yield []V and error, terminating at the first error encountered
  2. Collect2 (or simply .Collect() on an itx.Iterator2) will yield all errors: ([]V, []error)

@danail-branekov
Copy link
Member

Hey @BooleanCat

I saw that golang 1.23 is now out and I was eager to merge your pr. Unfortunately, latest staticcheck seems to have issues with golang 1.23 (even though it is supposed to support it) and that is failing our lint CI task... (sigh)

I have rebased your changes against latest main, resolved conflicts and replaced recently introduced references to go-functional v1 with v2. I have pushed that to a new branch, feel free to update your PR branch accordingly in order to get all the credits :)

@BooleanCat
Copy link
Contributor Author

Done and upgraded to beta.6

@danail-branekov
Copy link
Member

Almost there, @BooleanCat

With yesterdays release of staticcheck our linters are now happy. I have resolved the conflicts with the current main and fixed a linter issue by ammending the Replace remaining go-functional v1 usages with v2 commit in the (go-functional/v2)[https://github.com/cloudfoundry/korifi/tree/go-functional/v2] branch

Could you pick up those changes in your branch so that I can merge the PR?

@BooleanCat
Copy link
Contributor Author

Almost there, @BooleanCat

With yesterdays release of staticcheck our linters are now happy. I have resolved the conflicts with the current main and fixed a linter issue by ammending the Replace remaining go-functional v1 usages with v2 commit in the (go-functional/v2)[https://github.com/cloudfoundry/korifi/tree/go-functional/v2] branch

Could you pick up those changes in your branch so that I can merge the PR?

Done

@danail-branekov danail-branekov enabled auto-merge (rebase) August 15, 2024 10:15
@danail-branekov
Copy link
Member

danail-branekov commented Aug 15, 2024

A couple of api tests are failing as the result of collecting an empty iterator is nil, rather than an empty array. There is no reason to check for non-nil empty array, I will take care of those failures

By switching to go-functional v2 and adopting golang 1.23
iterators, collecting an empty iterator results into `nil` rather than
en empty slice. This makes no difference to Korifi, therefore this
commit removes explicit checks for empty non-nil slices in tests
@danail-branekov
Copy link
Member

OK, here is the commit that fixes the failures: d3ac8fb

@BooleanCat could you pick that up as well? Hopefully, this is goingto be the last iteration

auto-merge was automatically disabled August 15, 2024 10:43

Head branch was pushed to by a user without write access

@BooleanCat
Copy link
Contributor Author

OK, here is the commit that fixes the failures: d3ac8fb

@BooleanCat could you pick that up as well? Hopefully, this is goingto be the last iteration

All done

@danail-branekov danail-branekov enabled auto-merge (rebase) August 15, 2024 10:48
@danail-branekov danail-branekov merged commit 75a311b into cloudfoundry:main Aug 15, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants