Skip to content

Update to Swift 2#31

Merged
mdiep merged 20 commits intomasterfrom
swift-2
Sep 29, 2015
Merged

Update to Swift 2#31
mdiep merged 20 commits intomasterfrom
swift-2

Conversation

@neilpa
Copy link
Copy Markdown
Member

@neilpa neilpa commented Sep 17, 2015

No description provided.

Comment thread Commandant/Errors.swift
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Should we also enforce ClientError conform to ErrorType?

@jpsim
Copy link
Copy Markdown
Contributor

jpsim commented Sep 17, 2015

This is neat, @neilpa! There's already a Swift 2 branch at swift-2.0 (originally PR #26). These branches do a few things differently. There are places where this branch is better (or more up-to-date), but other cases where I prefer the approach in swift-2.0.

One of us should merge in improvements from the other branch. You or me, I'm happy either way!

@neilpa
Copy link
Copy Markdown
Member Author

neilpa commented Sep 17, 2015

D'oh, I didn't look beyond open PRs before starting these changes. Otherwise I would've built on top of your existing branch. I'll rebase this against the existing branch later today.

@neilpa
Copy link
Copy Markdown
Member Author

neilpa commented Sep 22, 2015

@jpsim Merged your changes into here as well.

@jpsim
Copy link
Copy Markdown
Contributor

jpsim commented Sep 22, 2015

Merged your changes into here as well.

Thanks! Could you change .travis.yml to point to osx_image: xcode7 so we can get CI back up?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should actually be split(2, ... as explained in #30.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

They must've changed it back because I didn't see any failures locally when running the tests.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, that's entirely possible.

@jpsim
Copy link
Copy Markdown
Contributor

jpsim commented Sep 22, 2015

Once CI is back up (and passing), this is good to go! Thanks @neilpa!

@mdiep
Copy link
Copy Markdown
Member

mdiep commented Sep 22, 2015

It looks like CI is failing because Box was removed from .gitmodules, but not from Carthage/Checkouts/.

@neilpa
Copy link
Copy Markdown
Member Author

neilpa commented Sep 22, 2015

Lets try this again.

@neilpa
Copy link
Copy Markdown
Member Author

neilpa commented Sep 22, 2015

Not sure why the tests aren't running on travis

  [Info] Collecting info for testables... (1642 ms)
  run-test CommandantTests.xctest (macosx10.11, logic-test)

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

The build has been terminated

@ikesyo
Copy link
Copy Markdown
Member

ikesyo commented Sep 23, 2015

It seems that we need xctool 0.2.6 and Quick/Quick#388.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This xcscmblueprint file should probably be removed and ignored. 👆

@jdhealy
Copy link
Copy Markdown
Member

jdhealy commented Sep 24, 2015

Other little thing: two references to Box still in Xcode project: lines 65 and 127.

@ikesyo
Copy link
Copy Markdown
Member

ikesyo commented Sep 28, 2015

The tests are passed now on this branch. 🎉

@jpsim
Copy link
Copy Markdown
Contributor

jpsim commented Sep 28, 2015

Looks good to me!

@jpsim
Copy link
Copy Markdown
Contributor

jpsim commented Sep 28, 2015

Actually, I think the Components.plist needs to be updated now that Box is now longer used.

Comment thread Cartfile.private
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can be set to Quick ~> 0.6?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Quick/Quick#388 is not included in the latest release.

@mdiep
Copy link
Copy Markdown
Member

mdiep commented Sep 29, 2015

This looks good to me apart from the concerns about the Cartfile.

@ikesyo
Copy link
Copy Markdown
Member

ikesyo commented Sep 29, 2015

Actually, I think the Components.plist needs to be updated now that Box is now longer used.

Is that the Carthage's one?

@mdiep
Copy link
Copy Markdown
Member

mdiep commented Sep 29, 2015

Since the upstream dependencies haven't had releases yet, I'm good to merge this. But we should be sure to get updated releases for those dependencies before doing a Commandant release.

mdiep added a commit that referenced this pull request Sep 29, 2015
@mdiep mdiep merged commit 468bbd0 into master Sep 29, 2015
@mdiep mdiep deleted the swift-2 branch September 29, 2015 01:52
@mdiep
Copy link
Copy Markdown
Member

mdiep commented Sep 29, 2015

Thanks for all the hard work everyone! 💖

@jdhealy
Copy link
Copy Markdown
Member

jdhealy commented Sep 29, 2015

👏

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.

6 participants