-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
allow construction of Set from list of iterators: Set(itr...) #22477
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
Conversation
|
I feel like it will lead to subtle bugs. Set() === Set()
Set([1]) === Set([1])
Set([1], [1]) === Set([[1]])
Set(x...) === ? # type-unstable
We could do the same with |
|
Is it really (technically) type unstable though? But I agree this can be confusing that the behavior is different for 1 or more elements. I just found it a bit frustrating that it was not possible pass directly the elements (when more than 1) as arguments of |
|
The difference is that it's less of an issue for |
f(x) = Set(x...)
@code_warntype f([[1,2]])Granted, one would presumably not write |
|
This is how we used to do this and we changed it intentionally so that all collections are constructed by passing them an iterable which yields the elements to be contained. You have to pick one since otherwise |
|
One option would be for the constructor to take any number of iterables and then include all of the things all of them yield. The fact that numbers are iterable would mean that |
I really like this idea, which enables the multi-arguments constructors by extending the 1-arg version in a regular way, thanks! |
base/intset.jl
Outdated
| very large integers), use [`Set`](@ref) instead. | ||
| # Examples | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jldoctest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good idea, will do.
|
Stefan's idea seems pretty solid to me, I don't see any drawback to adding the possibility to have multiple iterators in the constructor. Would anyone be opposed to this change? this would be ready to go on my end. |
Or |
I'm not chocked by such a behavior:
|
|
I think the real problem with the idea is that people will see code that does |
|
I find this a bit confusing, and I also dislike that it only applies to sets. We should only do this if we want all collection constructors to work like this (tricky, since it still conflicts somewhat with Dict constructors). Instead, we could add a vararg method to |
|
There is something similar for arrays with
This will be subjective at that point! but I find it confusing to indeed have |
|
I still find it bizarre for |
|
Closing, since the benefit is not huge and the current trend is to use constructor syntax to specify different kinds of initialization, so at the very least we should wait that the general design for collection constructors matures before going with thiss. |
A similar constructor exists for
Dict. This allowsI'm not sure if it's best to define this for each concrete type (i.e.
Set,IntSet, ...) or forAbstractSetas I did here.