-
Notifications
You must be signed in to change notification settings - Fork 150
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
Updating to RxSwift 5. #195
Conversation
Generated by 🚫 dangerJS |
Seems to be conflicting @LucianoPAlmeida |
Hey @freak4pc :) |
Can you rebase on top of master? |
Becuase you're doing a merge and I'm trying a rebase :) |
Huum sorry, didn't notice, rebasing :) |
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.
Good work! Just a few quick notes.
Action.podspec
Outdated
@@ -20,8 +20,8 @@ Pod::Spec.new do |s| | |||
s.source_files = "Sources/**/*.{swift}" | |||
|
|||
s.frameworks = "Foundation" | |||
s.dependency "RxSwift", "~> 4.0" | |||
s.dependency "RxCocoa", "~> 4.0" | |||
s.dependency "RxSwift", "~> 5.0.0" |
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.
Let's do ~> 5.0
everywhere
Tests/ActionTests/ActionTests.swift
Outdated
@@ -31,8 +31,8 @@ class ActionTests: QuickSpec { | |||
action.inputs.bind(to: inputs).disposed(by: disposeBag) | |||
scheduler.start() | |||
XCTAssertEqual(inputs.events, [ |
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.
FYI you can do
Recorded.events([
.next(...),
.next(....)
])
I'd actually prefer you'd do it for readability if possible.
Tests/ActionTests/ActionTests.swift
Outdated
next(20, "b"), | ||
completed(20), | ||
Recorded.next(10, "a"), | ||
Recorded.completed(10), |
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.
misaligned
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.
Thought swiftlint autocorrect would fix it :(
Tests/ActionTests/ActionTests.swift
Outdated
next(20, "b"), | ||
completed(20), | ||
Recorded.next(10, "a"), | ||
Recorded.next(10, "a"), |
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.
misaligned
Tests/ActionTests/ActionTests.swift
Outdated
it("first element receives single value") { | ||
XCTAssertEqual(element.events, [ | ||
next(30, "a"), | ||
completed(30), | ||
Recorded.next(30, "a"), |
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.
misaligned
Tests/ActionTests/ActionTests.swift
Outdated
next(30, "a"), | ||
completed(30), | ||
Recorded.next(30, "a"), | ||
Recorded.completed(30) |
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.
misaligned
Tests/ActionTests/ActionTests.swift
Outdated
it("second element fails with notEnabled error") { | ||
XCTAssertEqual(secondElement.events, [ | ||
error(20, ActionError.notEnabled) | ||
Recorded.error(20, ActionError.notEnabled) |
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.
misaligned
Tests/ActionTests/ActionTests.swift
Outdated
error(10, ActionError.notEnabled), | ||
error(20, ActionError.notEnabled), | ||
Recorded.error(10, ActionError.notEnabled), | ||
Recorded.error(20, ActionError.notEnabled) |
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.
misaligned
Tests/ActionTests/ActionTests.swift
Outdated
it("element fails with underlyingError") { | ||
XCTAssertEqual(element.events, [ | ||
error(10, ActionError.underlyingError(TestError)), | ||
error(20, ActionError.underlyingError(TestError)), | ||
Recorded.error(10, ActionError.underlyingError(TestError)), |
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.
misaligned
I need to resolve this on master first: #193 |
Sure @freak4pc, but anything left on this one? |
There will be issues here once I merge this because we removed InputSubject, for example. You'll have to rebase again after, hopefully It'd be fine :) |
Nice @freak4pc, thank's for the review :)) |
@LucianoPAlmeida I think that would do it. Can you rebase one final time on master and resolve any conflicts ? Thanks. |
@freak4pc Rebase done \o/ |
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.
LGTM! Thanks.
@LucianoPAlmeida Mind paying back with a review? ^_^ |
Sure :)) |
Already got a review there haha. Thank you! |
Haha no problem, feel free to ping me anytime. I’ll be happy if I can help :) Sent with GitHawk |
Just updating to the new version :))