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

Fixed the issue #88 -- when the route changes where the old route is … #119

Merged
merged 2 commits into from
Jul 26, 2019

Conversation

kmmraj
Copy link
Contributor

@kmmraj kmmraj commented Jul 11, 2019

Fixed the issue #88 -- when the route changes where the old route is longer than the new route

Refer to the issue#88 for the complete explanation of the issue.
This issue is fixed in ReKotlin-Router as well, refer here for more details.

@codecov-io
Copy link

codecov-io commented Jul 11, 2019

Codecov Report

Merging #119 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage    85.5%   85.57%   +0.07%     
==========================================
  Files           5        5              
  Lines         200      201       +1     
==========================================
+ Hits          171      172       +1     
  Misses         29       29
Impacted Files Coverage Δ
ReSwiftRouter/Router.swift 84.76% <100%> (+0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86af78f...0b722dc. Read the comment docs.

@kmmraj
Copy link
Contributor Author

kmmraj commented Jul 25, 2019

Can we get the PR reviewed?

@@ -119,6 +119,42 @@ class ReSwiftRouterUnitTests: QuickSpec {
expect(action1Correct).to(beTrue())
expect(action2Correct).to(beTrue())
}

it("generates a Change action on the last common subroute, also for routes of different length, old route is longer than the new one") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite a mouthful :) I suggest to rename the three cases to:

    it("generates a Change on the last common subroute for routes of same length") { ... }
    it("generates a Change on the last common subroute when new route is longer than the old route") { .. }
    it("generates a Change on the last common subroute when the new route is shorter than the old route") { ... }

Maybe you can come up with a shared context that fits, like context("changing non-root elements") or so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DivineDominion - changed the test names as you have suggested.

@DivineDominion DivineDominion merged commit 0b75f47 into ReSwift:master Jul 26, 2019
@peril-reswift
Copy link

peril-reswift bot commented Jul 26, 2019

@kmmraj Thanks a lot for contributing to ReSwift! We've invited you to join
the ReSwift GitHub organization – no pressure to accept! If you'd like more
information on what that means, check out our contributor guidelines.

Generated by 🚫 dangerJS

@DivineDominion
Copy link
Contributor

Merged. Thanks for the PR! @kmmraj

@kmmraj
Copy link
Contributor Author

kmmraj commented Jul 26, 2019

@kmmraj Thanks a lot for contributing to ReSwift! We've invited you to join
the ReSwift GitHub organization – no pressure to accept! If you'd like more
information on what that means, check out our contributor guidelines.

Generated by 🚫 dangerJS

I will be more than happy to join.

@rglearns
Copy link

@DivineDominion Since this is a subtle change fundamental to routing , can we consider creating a new release , so the change is reflected , when I install ReSwift via Cocoapods?

@DivineDominion
Copy link
Contributor

@rglearns Working on in in #117 👍

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.

4 participants