Skip to content

Conversation

@hlineholm
Copy link
Contributor

  • Remove use of segment as it actually refers to route element
  • Shorten names in order to increase readability without loosing unambiguity

@codecov-io
Copy link

codecov-io commented Oct 14, 2018

Codecov Report

Merging #107 into master will not change coverage.
The diff coverage is 63.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #107   +/-   ##
=======================================
  Coverage   85.57%   85.57%           
=======================================
  Files           5        5           
  Lines         201      201           
=======================================
  Hits          172      172           
  Misses         29       29
Impacted Files Coverage Δ
ReSwiftRouter/NavigationState.swift 100% <ø> (ø) ⬆️
ReSwiftRouter/Routable.swift 0% <0%> (ø) ⬆️
ReSwiftRouter/Router.swift 84.76% <79.16%> (ø) ⬆️

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 8269008...c3c8c31. Read the comment docs.

@ghost
Copy link

ghost commented Oct 14, 2018

Should be squashed...

@mjarvis
Copy link
Member

mjarvis commented Oct 15, 2018

@hlineholm When making breaking changes like this (renaming, changing, or removing public members), we should try to build a habit of adding the new stuff, and then marking the old ones as deprecated (& pointing to the new items)

One can deprecate the old items here like this:

@available(*, deprecated, message: "Deprecated in favor of `RouteElement`") public typealias RouteElementIdentifier = RouteElement

This way we can push out a minor update and allow users to migrate before a major release with breaking changes.

@ghost
Copy link

ghost commented Oct 16, 2018

I'll add it!

@DivineDominion
Copy link
Contributor

@hlineholm-flir Do you want to update this to the current master so we can merge this in?

Copy link
Contributor

@DivineDominion DivineDominion left a comment

Choose a reason for hiding this comment

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

Please add deprecation notices for the renaming (as @mjarvis noted in #107 (comment)) can be skipped since we have breaking changes anyway

@DivineDominion
Copy link
Contributor

@hlineholm We've merged #116 with a similar intent, so this would be a great addition in the same vein. You could even skip the deprecation notices since there's breaking changes scheduled for v0.7 anyway. Could you rebase your branch or merge master into it?

@DivineDominion
Copy link
Contributor

@hlineholm / @hlineholm-flir Are you still interested in updating the PR?

@hlineholm
Copy link
Contributor Author

hlineholm commented Aug 1, 2019

Yes @DivineDominion I am. Haven't had time to work on this lately so might be good if someone takes a look at the changes to see that I'm not missing anything. I've rebased it on latest master and in that process I discovered I'd missed some renaming in a test and in the README.md.

@DivineDominion
Copy link
Contributor

Thanks! Looks good to me. I'll request another pair of eyes :)

@djtech42
Copy link
Member

djtech42 commented Aug 7, 2019

Looks good!

@DivineDominion DivineDominion merged commit 5231c3e into ReSwift:master Aug 7, 2019
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.

5 participants