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

Remove unused problematic method #1460

Closed
wants to merge 1 commit into from
Closed

Remove unused problematic method #1460

wants to merge 1 commit into from

Conversation

AlekSi
Copy link
Contributor

@AlekSi AlekSi commented Jan 9, 2024

When the tools.go way of managing dependencies is used, there are occasional problems with incompatible transitive dependencies. Currently, Task suffers from one.

golang/go#61374 changed the signature of the slices.SortFunc function to make it compatible with Go 1.21. Currently, Task uses the old version. That makes it impossible to use Task and another tool that uses a newer version of golang.org/x/exp/slices in the same tools.go file.

Given that OrderedMap.SortFunc is not used anywhere, I propose just to remove it. This way, Task can be used with the latest version of golang.org/x/exp/slices and with the one currently specified in Task's go.mod file.

@pd93
Copy link
Member

pd93 commented Jan 9, 2024

Why not just update the golang.org/x/exp/slices package to the latest version instead? When 1.22 releases next month, we will probably drop support for 1.20, migrate to the standard slices package and drop x/exp entirely. Removing a method from our API doesn't seem worth it IMO - especially for the sake of compatibility with an experimental package that probably shouldn't be used in production anyway.

I've been wanting to remove the dep on x/exp for a while since the API is fragile and prone to issues like this. When I added it, I just wanted to try out all the new functionality, but with hindsight, I think this was the wrong thing to do in an open-source project. In the future, I think it's probably smarter for us to bring any functions we need from the x/exp package under our own internal package until something stable is released.

@andreynering
Copy link
Member

Hi @AlekSi,

I upgraded the package at #1462.

@AlekSi AlekSi deleted the exp branch January 11, 2024 08:22
@AlekSi
Copy link
Contributor Author

AlekSi commented Jan 15, 2024

I would really appreciate if you tag a new release with that fix :)

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