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

Apply migration to Swift 4 #160

Closed
wants to merge 1 commit into from
Closed

Apply migration to Swift 4 #160

wants to merge 1 commit into from

Conversation

damien-rivet
Copy link

@damien-rivet damien-rivet commented Jan 11, 2018

Implements #158.

Changelog

Added Swift version file.
Moved Swift version to Project instead of Targets.
Updated dependencies.
Fixed SwiftLint errors.
Splitted Router tests between multiple files.
Renamed some variables in tests.

@damien-rivet damien-rivet mentioned this pull request Jan 11, 2018
@devluckybot
Copy link

devluckybot commented Jan 11, 2018

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@damien-rivet
Copy link
Author

Sorry for the multiple commits, I didn't expect Travis to complain that much...

Added Swift version file.
Moved Swift version to Project instead of Targets.
Updated dependencies.
Fixed SwiftLint errors.
Splitted Router tests between multiple files.
Renamed some variables in tests.
@codecov-io
Copy link

Codecov Report

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

@@           Coverage Diff           @@
##           master     #160   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files          10       10           
  Lines         481      481           
=======================================
  Hits          479      479           
  Misses          2        2

@damien-rivet
Copy link
Author

Hi @MP0w,

Will you be able to review my PR at some point?

Thanks in advance for your answer.

Copy link
Member

@MP0w MP0w left a comment

Choose a reason for hiding this comment

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

Ty for the PR, I see you moved around some specs and eventually added some. Could you separate it from the swift 4 PR (opening another PR) and explain the reason? Thanks


@testable import Kakapo

class RouterMultipleTests: QuickSpec {
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these specs already covered in the Router specs?

Copy link
Author

Choose a reason for hiding this comment

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

They were, I haven't added new specs, I just splat the existing one into multiple files to simplify the reading and to prevent SwiftLint from failing the build.

But if you prefer this to come from another PR, I think I should be able to make it happen.

@joanromano
Copy link
Member

Hey @Ethenyl @MP0w sorry didn't get to this in time - I believe we should still attempt to merge it; can I help? Do you want me to review it?

@damien-rivet
Copy link
Author

Hi @joanromano,

It's been a while since I gave a look to this PR... If you want you can review it, I'll also try to upgrade to Swift 4.1 (if possible without breaking the CI) when I have some time.

@joanromano
Copy link
Member

joanromano commented May 28, 2018

Hi @Ethenyl I have checked the PR and I have some questions:

  • Some of the specs have some no-op changes (e.g from Dog to Labrador) - I don't think this applies to a Swift 4 migration PR
  • Some specs are moved and the organization of the specs, in general, is changed - again, I don't think this applies to a Swift 4 migration PR and I'd be happy to discuss it in another PR

Other than that I am happy to review and merge only changes corresponding to a Swift 4 migration.

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