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

Replace "Sequence" annotations by "Iterable" #247

Closed
wants to merge 2 commits into from

Conversation

roberfi
Copy link

@roberfi roberfi commented May 22, 2024

I have seen that Sequence annotations could be replaced by Iterable making the range of accepted types wider

In order to improve type annotations, I Sequence annotations can be replaced by Iterable. This would make all these methods available for dict, set or Generator, for example, with static type checking.

This is backwards compatible and just tests to check methods with dictionary as a Collection example where added.

@roberfi roberfi force-pushed the replace-sequence-by-iterable branch from 8224c3f to d2999d2 Compare May 22, 2024 19:01
@offbyone
Copy link
Member

The reason we stuck to sequence instead of iterable is that iterables can be consumed by the iteration, where sequences are not. We are able to make typed assertions about the immutability of them here, thus.

If you want the matchers you're implementing here, I recommend implementing them in a small library that depends on Hamcrest; feel free to use the helper classes and API, but I don't believe we'll take this feature.

Thank you, though! Your PR is appreciated!

@offbyone offbyone closed this May 22, 2024
@roberfi
Copy link
Author

roberfi commented May 23, 2024

Okay. Thank you so much for the explanation. I understand the logic behind the decision. And what about Collection? It's less restrictive than Sequence and the only difference between them is that Sequence is Reversible while Collection not. This would make Set suitable:
image

Maybe we could leave IsSequenceContainingInOrder with Sequence since Collection order ir not guaranteed.

@offbyone
Copy link
Member

In either case, we'd be leaving the other names, since those names are part of the API and I am not currently inclined to deprecate them. Adding aliases for them that support collections would be fine, removing the existing ones would not.

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.

2 participants