Adding OSX target #56
Conversation
…ation on OSX. Using a compiler directive to discard the code.
…rror.Cause (saying Cause isn't a member type of Error). Is an extension really necessary?
…g could be used furthermore in the UI section, but requires a lot more tricks.
…cannot be used on OSX without a lot more work.
# Conflicts: # Source/Request/FailedRequest.swift # Source/Resource.swift # Source/Support/Progress.swift
|
Wow, thanks for taking on this huge task! This is fantastic. I’d been wanting to do this for a long time, but had been hoping somebody who knows more about OS X development than me would take it on. Thank you! I have a new Siesta release coming soon, hopefully next week. My release strategy with this PR would be to get 1.0 beta 7 out the door first, then merge this and add OS X support in beta 8. Main thoughts below; I’m also adding some minor notes in the code.
Long story short: I’ll make sure that this PR and fix for #31 go in the same release. I’m surprised that the typealias approach works as well as it does. I assumed that ResourceStatusOverlay would have to be totally iOS-specific. Have you sanity checked that your code does in fact work property on OS X? If so, fantastic and wow. I’m on the fence about how the typealiases should be named. The Swift API guidelines encourage us to move away from the prefixes like
Whatever name we choose will show up in docs and autocomplete, and I don’t want users to get confused by what they see. Is there a common practice you’ve seen in other Swift frameworks that target both OS X and iOS? Re Nocilla: I’ve been thinking about dropping it. It’s buggy and full of race conditions, and the maintainer still hasn’t accepted that ancient PR. If you have other HTTP mocking suggestions that work on both platforms, I’m all for it! That shouldn’t hold up this PR, but let me know if you have a preference. In the meantime, if you submit a PR against my Nocilla PR for the OS X support, I can merge it. |
| ); | ||
| name = "Test-only"; | ||
| name = Frameworks; |
There was a problem hiding this comment.
Did you just delete the “Test-only” subgroup? That’s fine, I suppose, now that Alamofire is optional.
There was a problem hiding this comment.
Yes, I deleted the Test-only subgroup. I really wanted to keep things as exactly as it was, but it took quite a few steps to make sure I had the right frameworks all named the same in the right (build) places. And this subgroup just got deleted. That's the only subgroup deletion I made I think.
There was a problem hiding this comment.
I think that’s just fine. It was a minor style point; if it caused any trouble at all, it’s just not worth it.
There was a problem hiding this comment.
I have a fix for this that also cleans up some other code. Can you give me push permission for your Siesta fork?
There was a problem hiding this comment.
Sure! You should have received an invite by now.
There was a problem hiding this comment.
Thanks! Just realized I posted my comment on the wrong line note. The fix is for the Error.Cause extension. Now pushed.
|
Thanks for your great response to the PR. I need to make some more tests on my OSX projects, but so far it works. As for the prefixes, I agree. I used "BOS" since that's the one I found in your code. But if you prefer a more streamlined-swifty way, we go for it! In my other Swift projects I also removed all the prefixes, but that's for types I own. UIKit classes continue to hold their prefix for instance. And since we have here typealiases... As for Nocilla, I would agree to completely remove it. Actually, the fewer dependencies the better. I have some troubles running the tests right now, but I'll try to find another way. I realise there is quite a bit of work left. Although I am enthusiastic to see this PR pushing things forward, it will require some more time. |
|
The
There’s a swift-evolution proposal to remove the
Yes, but you’ve done a tremendous amount, and things are already looking pretty good here. |
|
I have some troubles making the ResourceStatusOverlay work. And wondering... is this really the core of Siesta? It adds a lot of troubles (in addition of me not understanding how the xib is really hooked up). But I'd love to keep the logic of it, with state changes. One just need to remove the UI part. At least, for OSX the whole coverParent stuff is not really useful anyway. |
|
Yes, I suspected it might be hard to get that code working on both platforms at once. Thanks for giving it a try. I had loose plans to separate the UIKit stuff into a separate module — either within this repo or as a separate library altogether. In addition to the cross-platform reasons, it will also make things better for SwiftPM. I already have some sketches of splitting the purely logical state transition stuff in However, all of that is a big, messy change, and I don’t want this PR to snowball. How about this revised plan?
Follow-ups would be:
Of those, I think 1 and 2 should be done before releasing a beta with OS X support, but 3 and 4 could be done at leisure. |
|
I'm glad to basically agree with everything. I will push the small changes to merge the PR asap. |
|
I made the changes, which means basically getting rid of all the OSX work I did at first. But that's life. :-) I deleted the OSX tests target as well, as it doesn't make real sense to keep all typealiases just to run the tests (I must say I haven't read much the details of the tests, just that UIViews are used). I also had to keep the typealias for (NS/UI)Image, because it is used in the ImageResponseTransformer. |
Hopefully some of that investigation you did does pay off in the future, though, when building OS X UI helpers.
No, we should keep the OS X tests! I definitely don’t want to have an untested target in the project. The only usages of UIView in the specs were as dummy objects (something that can’t be transformed by JSON, for example). You should be able to replace all of those with some arbitrary class from Foundation that wouldn’t normally show up in a response entity. Feel free to revert 0ccfb2f & force push to fix that.
Right, of course. |
|
Ok I will put osx tests back |
|
OK. I didn’t want to remove that commit from here because I don’t want to mess up your local git history — but once 0ccfb2f is gone, I can fix up the usages of UIView in the specs if you want. We’re close! |
|
I'm glad I made an atomic commit on that matter. So I did a revert, and changed UIView classes for NSData in tests. On my side, I can run both iOS and OSX tests, and they all pass. Yes, we're close! |
|
Hm, Travis says it fails. But I couldn't find the error. |
|
Got it. Yesterday I renamed the iOS scheme without changing the .travis file. Bad idea. I reverted to the old scheme name. |
|
BTW, https://github.com/AliSoftware/OHHTTPStubs seems a good candidate to ditch out Nocilla. |
It may be hiding useful info about sporadic build failure.
|
I think we’re there! Given that feedback on the pipeline API has been slow and this has moved fast, I’m going to cut a release with this instead and move the pipeline stuff back to a subsequent beta. Your scheme renaming was a good idea. I checked some other multi-platform projects, and they do the same. I renamed the targets too, and got the travis file updated. (Don’t look in there. It’s horrid.) Found a race condition in one of the specs. Don’t know if it’s a timing difference on OS X, or if it allows more simultaneous connections per host by default and that's what caused it. Regardless, one spec was failing about 3% of the time on OS X. Now fixed. I’m still seeing sporadic build failures from Travis … but I blame Travis itself. Its Xcode environment has all kinds of weird timeout issues. Failed builds almost always pass if I rerun them. It’s possible this might be an issue with Siesta and not Travis, but I never have these problems locally. |
|
And we’re there! Merci bien @onekiloparsec for getting this through! |
|
Avec plaisir! If I have time, I'll keep an eye on the Nocilla replacement. |
Hi,
This PR is about adding an OSX target. After having looked around and tested various things, I think siesta is one of the best candidates for the app I am preparing. But it's an OSX app. I need an OSX target...
This PR introduces the OSX target. Apart from all UIKit stuff, there are 0 changes (and I've merged upstream). The problem of course is UIKit's stuff. Fortunately it is fairly easy to workaround, since many many classes share the same APIs between the various platforms.
For the cases where it is different, I've written a OSX.swift compatibility file where typealiases are defined (they allow to use the same type everywhere, mapping to the correct one depending on platform). And where I've written the little tricks necessary for the iOS siesta code to NOT change. That is, the tricks are here to adapt OSX interfaces to iOS ones, since I guess iOS remains the main ... target. :-)
I've been able to build an OSX Test target as well. However, given the various dependencies on other thing to build tests, this makes things quite a lot more difficult. In particular, I had to create an OSX target for Nocilla (and change the file Cartfile.private to point to it)
Anyway, I think the constraint of an OSX target is a good occasion to clearly separate class/code concerns/responsabilities. This PR is created maybe a bit toot early, but I wanted to expose/discuss it before siesta reach 1.0, to raise awareness. I am happy to discuss it for any changes and improvements necessary before merge, of course!
Thanks anyway for the great project.