-
Notifications
You must be signed in to change notification settings - Fork 40
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
Nested NonEmpty
#46
base: main
Are you sure you want to change the base?
Nested NonEmpty
#46
Conversation
I just realized I forgot to add tests for |
Turns out it's quite easy to make |
@RemiBardon Just wanted to chime in to let you know we saw this, and thanks for exploring! We won't have time to dig in quite yet but hopefully soon. |
No problem, I'm busy too 😉 I really tried to work around my problem, but I couldn't find a clever way to do it without storing more than the initial collection in memory 😕 |
2618e9c
to
675668b
Compare
675668b
to
48bb120
Compare
I finally made it! It works like a charm 🙂 The biggest issue, which I tried hours to avoid, but couldn't get around, is that I have to store one I should have noted everything I tested, but my main problem was that because of how Swift's type system works, I couldn't create a |
@stephencelis This PR is now ready 🙂 |
I found a bug, I'm fixing it. For some reason, fileprivate typealias AtLeast11<C: Swift.Collection> = NonEmpty<AtLeast10<C>>
extension AtLeast11 {
public var drop10: NonEmpty<Collection.SubSequence> {
try! NonEmpty<Collection.SubSequence>(from: self.rawValue.dropFirst(10))
}
} |
This also means |
Also remove `.drop10`, as I could not make it type safe.
After hours of searching how to make |
Nested I'm adding them. |
b1a422a was a long commit… I had to change many things 😕 I see a few problems here:
Now that it works, I will try to refactor it in two steps:
|
The main problem with this is that I still have to separate two cases:
The only way I can think of to separate these cases, and keep a single definition of protocol conformances, is to go back to the I will try to refactor this way, and I'll see where it takes me. |
I'll probably do one more step, which is to make |
I feel stupid… I realize I can't have type safe nesting if I'm using enums 🤦🏻♂️ |
I finally got it! Separating /// - Note: We need to separate `WithMinimumCount` from `NonEmptyProtocol`
/// to avoid the "Self or associated type requirement".
public protocol WithMinimumCount {
static var minimumCount: Int { get }
} |
@stephencelis I marked this pull request as ready to review, as I think it has enough tests, which all pass, covering every requirement I had in mind. I will start using it myself on my branch, to see if problems arise. |
One thing I would like to add, but I'm not sure if it's a good idea, is an extension saying |
00595e9
to
5807a16
Compare
5807a16
to
eae3484
Compare
I just force pushed twice to cleanup some things I had changed and shouldn't have. I can also squash my commits if you want to. |
Oh, and by doing so, I was able to avoid storing |
I updated Xcode and the toolchain, and I get It happens with:
Let me fix it then… Edit: Until then, I reported https://bugs.swift.org/browse/SR-16024. |
It's too complex for the compiler, we have to remove it. It's probably useless anyway.
I removed I propose we'll try to find a solution only if someone needs it. |
Following this discovery, I realised I could revert some changes I made in the code, to make the Pull Request as clean as possible. |
There are a lot of things I changed which I could have avoided. To make Pull Request more readable, I preferred to revert them.
6a75a26
to
eeb53fd
Compare
Converting this Pull Request to draft once again, because by using it I realized it introduces a huge increase in compile time (see https://forums.swift.org/t/wrong-redundant-conformance-constraint-warning/56207/15). I will mark this PR as "ready" once I fix this issue. |
Worth sharing here: https://twitter.com/slava_pestov/status/1507537318315450370 |
A new trunk development snapshot was released, and now everything (including I don't mark the PR as "ready", as you might want to wait for the compiler have a stable release containing the fix. I will use my branch personally, and wait for your approval on this PR. |
Following our discussion #45, here is a pull request introducing my proposed ideas.
It doesn't break any existing test, and adds new ones for introduced features.
Let me know what you think 🙂