Skip to content

Support 1.23 iterator for set #140

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

Merged
merged 4 commits into from
Mar 4, 2025
Merged

Support 1.23 iterator for set #140

merged 4 commits into from
Mar 4, 2025

Conversation

amikai
Copy link
Contributor

@amikai amikai commented Aug 2, 2024

This PR use to discussion.

@freeformz
Copy link

FWIW: I'd really like to see this.
If this is stalled I'm happy to open a new PR.

@deckarep
Copy link
Owner

@freeformz - a fresh PR would be nice. Please factor in the discussions up until now and I'll make a commitment to review it and merge of it if I can get a sign input by a few folks.

@amikai
Copy link
Contributor Author

amikai commented Mar 3, 2025

Hi @freeformz, @deckarep. The PR was in draft because it’s still under discussion in #141. If @deckarep thinks that the discussion is finalized and a decision is reached, feel free to comment or review the PR

@amikai amikai marked this pull request as ready for review March 3, 2025 15:25
@deckarep
Copy link
Owner

deckarep commented Mar 3, 2025

@amikai and @freeformz - it looks like you both want your PR’s merged on Iterator support for this package.

I appreciate both of your efforts on this but both PR’s are quite different. freeformz looks to be a significant change at the moment while amikai’s is much smaller in scope and looks to be much more straightforward.

Let me be be clear: I want to support iterators but when this was initially proposed it felt too early to just jump in without a better understanding of the best way forward. This is why I didn’t rush to merge it. It’s also why I don’t rush to merges everyone’s changes.

Some significant projects are using this package and any abrupt changes which cause confusion, frustration will cause these projects to quickly fork and/or abandon this repo.

So as it stands I’m interested in taking the least breaking changes, with the least scope currently. Additionally, I want to ensure that methods are deprecated overtime and not overnight.

So with all of that said, @amikai - it seems like the last thing we had to come to agreement on was just the naming of the iterator method? And it looks like Values should work just fine?

@freeformz - your input is still welcome but I want to introduce smaller scopes changes at this time. As more people migrate to later versions of Go I will consider cutting a V3 release.

@freeformz
Copy link

No worries, all good. I am just really motivated to have iterator support atm so I don't really care whose implementation is accepted.

I'll close my pr.

TY!

Copy link
Owner

@deckarep deckarep left a comment

Choose a reason for hiding this comment

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

LGTM

@deckarep deckarep merged commit 0d26f4c into deckarep:main Mar 4, 2025
15 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