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

Renamed Argument Labels #116

Merged
merged 7 commits into from
May 25, 2019
Merged

Renamed Argument Labels #116

merged 7 commits into from
May 25, 2019

Conversation

TaeJoongYoon
Copy link
Member

I edit functions in Router name and arguments according to API Design Guidelines

// From
routingActionsForTransitionFrom(_ oldRoute:,newRoute:)
// To
routingActionsForTransition(from oldRoute:,to newRoute:)

// From
routableIndexForRouteSegment(_ segment:)
// To
routableIndex(for segment:)

@DivineDominion DivineDominion mentioned this pull request May 20, 2019
6 tasks
@mjarvis
Copy link
Member

mjarvis commented May 21, 2019

@TaeJoongYoon Thanks for contributing!

When we rename or remove any public facing interfaces, we need to provide deprecated forwards from the old functions to avoid breaking backwards compatibility.

[Edit]: Looks like we have breaking changes planned for v0.7.0 anyways so no need to add deprecation markers. Sorry!

@DivineDominion
Copy link
Contributor

@TaeJoongYoon Travis reports Use of unresolved identifier 'routableIndexForRouteSegment' -- have you overlooked an occurence during the renaming process?

@TaeJoongYoon
Copy link
Member Author

@mjarvis
It's okay! :) If needed, I will add it.

@DivineDominion
I'm sorry. I missed it one line. I edited it and commit it!

@DivineDominion
Copy link
Contributor

@TaeJoongYoon There seems to be more typos :)
https://travis-ci.org/ReSwift/ReSwift-Router/jobs/535598154#L2080

The for: is missing. (I suggest you ⌘U run tests locally too to catch this)

@mjarvis
Copy link
Member

mjarvis commented May 22, 2019

It would also be beneficial to add an entry to Changelog.md referencing these breaking changes

@TaeJoongYoon
Copy link
Member Author

@DivineDominion I'm sorry.. test succeed.

@mjarvis Do I add it?

@DivineDominion
Copy link
Contributor

@TaeJoongYoon Great!

Regarding the CHANGELOG: yes, please! For example:

- Renamed argument labels for modern Swift conformance (#116) - @TaeJoongYoon

Or whatever you find fitting.

@TaeJoongYoon
Copy link
Member Author

@DivineDominion I added it with comments!

@codecov-io
Copy link

codecov-io commented May 23, 2019

Codecov Report

Merging #116 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #116   +/-   ##
======================================
  Coverage    85.5%   85.5%           
======================================
  Files           5       5           
  Lines         200     200           
======================================
  Hits          171     171           
  Misses         29      29
Impacted Files Coverage Δ
ReSwiftRouter/Router.swift 84.66% <100%> (ø) ⬆️

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 c9e9173...7662501. Read the comment docs.

Changelog.md Outdated
@@ -5,6 +5,9 @@

- Removes the compatibility of this with `ReSwift-Recorder`, which itself is being deprecated.
- Ensures compatibility with latest versions of `ReSwift` which have removed these types.
- Renamed argument labels for modern Swift conformance ([#116](<https://github.com/ReSwift/ReSwift-Router/pull/115>)) - @TaeJoongYoon
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Renamed argument labels for modern Swift conformance ([#116](<https://github.com/ReSwift/ReSwift-Router/pull/115>)) - @TaeJoongYoon
- Renamed argument labels for modern Swift conformance ([#116](<https://github.com/ReSwift/ReSwift-Router/pull/116>)) - @TaeJoongYoon

(fixed the pull request number in the URL)

Copy link
Member Author

Choose a reason for hiding this comment

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

My apology for the typo...

Copy link
Member Author

Choose a reason for hiding this comment

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

Do I have to do anything extra?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see anything else. I'll merge it once this is fixed 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. I fixed it :)

@DivineDominion DivineDominion merged commit 86af78f into ReSwift:master May 25, 2019
@peril-reswift
Copy link

peril-reswift bot commented May 25, 2019

@TaeJoongYoon 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

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