search

nerdishbynature / octokit.swift

A Swift API Client for GitHub and GitHub Enterprise
MIT License
490 stars 126 forks source link

Make Octokit build with Linux #69

Closed f-meloni closed 5 years ago

f-meloni commented 5 years ago

I was trying to use octokit.swift as Github client on Danger-swift (PR https://github.com/danger/swift/pull/95 )

But i discovered that octokit.swift doesn't compile on the Linux environment

https://travis-ci.org/danger/swift/jobs/451464822

This PR solves the problem

https://travis-ci.org/danger/swift/jobs/451515014

To keep the objc compatibility i had to duplicate the models to be not @objc on Linux. Not sure this is the best possible approach, i would prefer to not have duplications obviously, i'm open to any suggestion or better ideas :)

pietbrauer commented 5 years ago

Hey, thanks for the PR.

I am also on the fence if removing the @objc is actually fine these days. It would be a breaking change, though. These issues weren't present on RequestKit when Tiago introduced the Linux build: https://github.com/nerdishbynature/RequestKit/pull/55/files.

Maybe you can have a look at the .travis.yml and make the same changes here too, so we have CI for Linux.

f-meloni commented 5 years ago

@pietbrauer It took some attempts but now it is running the CI on Linux as well.

Re: @objc i personally think that remove the objc compatibility would be the best option, even if it requires a new major release, but is your call :)

pietbrauer commented 5 years ago

Sweet, thanks a lot!

It's 2018, I created this 2015 when Swift was only 1,5 years old. I guess it is ok to have a Swift Only APIClient these days. Any opinions @phatblat @nwest? Or anyone else of the 11 people who read this?

f-meloni commented 5 years ago

Hey @pietbrauer, did you have a think about this? Should i remove the objc compatibility? :)

pietbrauer commented 5 years ago

Yeah, let's go for it.

f-meloni commented 5 years ago

@pietbrauer It should be fine now :) If you are happy with it, can you please merge and release? Then i can point the release from danger-swift? Thank you :)

pietbrauer commented 5 years ago

Nice work, thanks!