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

prefixset: add iterators over prefixes #13

Merged
merged 1 commit into from
Dec 20, 2024
Merged

Conversation

database64128
Copy link
Contributor

Closes #12.

@aromatt
Copy link
Owner

aromatt commented Dec 18, 2024

Thanks for this!

I looked for examples of iterators being added to existing packages with respect to naming, and didn't find anything.

I'm considering just using All() and AllCompact(), or even your original suggestion of repurposing Prefixes() and PrefixesCompact() since this package doesn't have many users yet.

I need to sign off for the evening but will follow up tomorrow.

@database64128
Copy link
Contributor Author

I looked for examples of iterators being added to existing packages with respect to naming, and didn't find anything.

There's one in the standard library: golang/go#61901. They went with Lines, Bytes for new functions, and SplitSeq, FieldsSeq for iterator forms of existing functions, with the rationale being:

If we were writing the library from scratch, we might use the names Split, Fields, and Runes for the iterator-returning versions, and code that wanted the full slice could use slices.Collect. But that's not an option here, so we add a distinguishing Seq suffix. We do not expect that new functions will use the Seq suffix. For example the function above is Lines, not LinesSeq.

I'm considering just using All() and AllCompact(), or even your original suggestion of repurposing Prefixes() and PrefixesCompact() since this package doesn't have many users yet.

Both options look good to me. I'll update the PR when you make a decision.

prefixset_iter_test.go Outdated Show resolved Hide resolved
@aromatt
Copy link
Owner

aromatt commented Dec 19, 2024

Both options look good to me. I'll update the PR when you make a decision.

Thanks for the examples. Since PrefixSet says what kind of element it stores right there in the name, I've decided to go with All and AllCompact.

This lets us keep parity with netipx's API for now, while moving forward with conventional names for iterators. It also nicely telegraphs names for PrefixMap iterators: All, Keys, and Values (plus respective *Compact versions).

@database64128
Copy link
Contributor Author

Since PrefixSet says what kind of element it stores right there in the name, I've decided to go with All and AllCompact.

Done.

This lets us keep parity with netipx's API for now, while moving forward with conventional names for iterators. It also nicely telegraphs names for PrefixMap iterators: All, Keys, and Values (plus respective *Compact versions).

I'll open another PR for PrefixMap iterators.

@aromatt
Copy link
Owner

aromatt commented Dec 20, 2024

@database64128 would you like to update the commit message with the name changes before I merge?

@database64128 database64128 changed the title Add PrefixSet.PrefixSeq and PrefixSet.PrefixCompactSeq prefixset: add iterators over prefixes Dec 20, 2024
@database64128
Copy link
Contributor Author

would you like to update the commit message with the name changes before I merge?

Oops, missed that. Done.

@aromatt
Copy link
Owner

aromatt commented Dec 20, 2024

Looks good!

@aromatt aromatt merged commit 53f67b6 into aromatt:main Dec 20, 2024
1 check passed
@database64128 database64128 deleted the iter branch December 20, 2024 06:10
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.

Add PrefixSet.PrefixSeq and PrefixSet.PrefixCompactSeq for Go 1.23+
2 participants