-
Notifications
You must be signed in to change notification settings - Fork 429
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
Add retry(upTo:interval:on:) #496
Conversation
var values: [Int] = [] | ||
var completed = false | ||
|
||
original.retry(upTo: Int.max, interval: 1, on: scheduler) |
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.
Please add a test that tests hitting the uptown
limit.
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.
Thank you for your review so much! I 'll add test!
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.
I added a test that tests hitting the upto
limit.
please see
https://github.com/ReactiveCocoa/ReactiveSwift/pull/496/files#diff-9cd786f9e52e786771d96cf8c42d3befR2018
|
||
context("with interval") { | ||
|
||
it("should send values at the given interval until hitting the limitation") { |
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.
This isn't a very accurate description of the test. It's testing the retrying, not testing the limitation.
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.
I see. I updated description of the test. 😄
var completed = false | ||
|
||
original.retry(upTo: Int.max, interval: 1, on: scheduler) | ||
.start { event in |
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.
This indentation is off. The ReactiveSwift style is to have methods like this be indented one stop.
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.
I fixed indentation 😄
Sources/SignalProducer.swift
Outdated
|
||
retries -= 1 | ||
return producer | ||
}.retry(upTo: count) |
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.
This indentation doesn't match the ReactiveSwift style. It should be indented like so:
return flatMapError { error in
…
}
.retry(upTo: count)
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.
Thank you for your review ! I 'll fix . 😄
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.
I fixed indentation.
Thanks for opening a PR! |
I updated pull request. Any thoughts? |
Sources/SignalProducer.swift
Outdated
retries -= 1 | ||
return producer | ||
} | ||
.retry(upTo: count) |
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.
This is not fully addressed yet.
return flatMapError { error in
…
}
.retry(upTo: count)
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.
Oh, sorry.. I 'll fix soon!
Sources/SignalProducer.swift
Outdated
retries -= 1 | ||
return producer | ||
} | ||
.retry(upTo: count) |
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.
Please indent the two lines above as well.
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.
Thank you for your review and feedback. I sent pull request again. Is this correct?
7a5b7b9
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.
Please add this to the CHANGELOG as well!
|
||
scheduler.advance(by: .seconds(1)) | ||
expect(count) == 3 | ||
expect(values) == [1, 2, 3] |
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.
Can we verify that the error
was sent as well?
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.
I got it.
I 'll try to add testing of error and update CHANGELOG.
thanks! 😄
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.
Can we verify that the error was sent as well?
I added test. How about this commit 73d2d4d ?
73d2d4d
to
aeee063
Compare
Thanks for the PR! Sorry I've been slow reviewing. It's been a busy few weeks. The Swift 4 linux build failed, but I think that's because it needs to be updated. |
Sorry for my late reply.. I deeply appreciate your review and feedback! 😄 |
Checklist
Summary
I implemented #224 and write test code
I'm new to this. Is this correct?
Please review in your free time.