Skip to content

[Swift] Support use in Framework#981

Merged
wing328 merged 3 commits into
swagger-api:develop_2.0from
Edubits:develop_2.0
Jul 21, 2015
Merged

[Swift] Support use in Framework#981
wing328 merged 3 commits into
swagger-api:develop_2.0from
Edubits:develop_2.0

Conversation

@Edubits

@Edubits Edubits commented Jul 15, 2015

Copy link
Copy Markdown
Contributor

As discussed in #901 I added explicit access control to classes, methods and members to support the use in a Framework, e.g. when using it in a CocoaPod.

cc: @tkqubo

Edubits added 2 commits July 15, 2015 18:20
To support use in a Framework set access level of relevant classes,
methods and members to public
@wing328

wing328 commented Jul 16, 2015

Copy link
Copy Markdown
Contributor

@Edubits thanks for the PR.

@tkqubo when you've time, I wonder if you can help review the change.

I'm no expert in Swift but the change looks ok to me as it simply makes the functions public

@Edubits

Edubits commented Jul 16, 2015

Copy link
Copy Markdown
Contributor Author

I've just added basic support for form data (just strings, not files/multi-part)

@tkqubo

tkqubo commented Jul 17, 2015

Copy link
Copy Markdown

Thanks @Edubits for this pr, it looks ok, and thanks again for fixing my bug.
It seems api method became class func. Doesn't this change make api class customization a little bit difficult on a client side? (e.g. one may want to extend swagger-generated api class, or add an extension to store some instance-specific state on api class itself)

@Edubits

Edubits commented Jul 17, 2015

Copy link
Copy Markdown
Contributor Author

@tkqubo Fair point, it's still possible to extend the class, e.g.:

extension SwaggerClientAPI {
    public class MyFooAPI : FooAPI {
        override public class func getBar() -> RequestBuilder<Bar> {
            NSLog("This will be printed")
            return super.getBar()
        }
    }
}

or (making the extension internal and different way of extending)

class MyFooAPI : SwaggerClientAPI.FooAPI {
    override class func getBar() -> RequestBuilder<Bar> {
        NSLog("This will be printed")
       return super.getBar()
    }
}

You can also make the overridden version internal. You're right about state variables on class level, this is not possible, but should it? If you really want you can add them and add a method which calls the class methods.

The reason I made the methods class-methods is that you need to have a public initializer to be able to use it otherwise in a Framework. So the choice was to either add an initializer (which does nothing), or make them class methods. As the API classes don't use state I chose the latter.

@tkqubo

tkqubo commented Jul 17, 2015

Copy link
Copy Markdown

@Edubits Thanks for explaining the intention of your implementation. I mostly agree with your rationale.

Yes, as you said normally the API classes don't use state (or should not use), and even a static method can be a member of inheritance tree in great Swift 😃

But one last thing I care a bit about is that if we are to write protocol-based code or test code in a DI fashion, API injection looks like this:

// Swagger generated code
class SwaggerClientAPI {
    class FooAPI {
        // ...
    }
}


// Client-side code
protocol FooAPIProtocol {
    // ...
}

extension SwaggerClientAPI.FooAPI: FooAPIProtocol { }

class FooAPIMock: FooAPIProtocol {
    // ...
}

class BarService {
    // ...
}

class FooService {
    let api: FooAPIProtocol.Type
    let barService: BarService

    init(api: FooAPIProtocol.Type = SwaggerClientAPI.FooAPI.self,
            barService: BarService = BarService()) {
        self.api = api
        self.barService = barService
    }
}


// let fooService = FooService()
//  or 
// let fooService = FooService(api: FooAPIMock.self)

Although the lines above will compile without any problem, switching the implementation using Type and self looks counterintuitive to me.

In my opinion, instance-level method inheritance might be comfortable with other possible frameworks and should be chosen if possible, even introducing an empty initializer.

But I'm not sure if Swift language goes well with this kind of DI like java does 😓

@Edubits

Edubits commented Jul 17, 2015

Copy link
Copy Markdown
Contributor Author

Although the lines above will compile without any problem, switching the implementation using Type and self looks counterintuitive to me.

I must agree with that. You can get about the same result in a little bit more simple way:

// Swagger generated code
class SwaggerClientAPI {
    class FooAPI {
        // ...
    }
}

class FooAPIMock: SwaggerClientAPI.FooAPI {
    // ...
}

class BarService {
    // ...
}

class FooService {
    var api = SwaggerClientAPI.FooAPI.self
    var barService = BarService()
}

// let fooService = FooService()
//  or
// let fooService = FooService()
// fooService.api = FooAPIMock.self

This version was based on the DI described on http://swiftandpainless.com/2015/04/stubbing-nsurlsession-with-dependency-injection/. This way lets the compiler do a bit more work, making the code a lot easier to read. What do you think? :)

@tkqubo

tkqubo commented Jul 18, 2015

Copy link
Copy Markdown

Oh, there is already an example to use a class itself as an instance variable and switch the class for test purpose. This looks fine, now I totally agree with your proposal.

Thanks again for giving me a nice article! @Edubits

@wing328

wing328 commented Jul 21, 2015

Copy link
Copy Markdown
Contributor

If no concern from anyone, I'll merge the PR into develop_2.0 branch later today.

@tkqubo

tkqubo commented Jul 21, 2015

Copy link
Copy Markdown

👍

@Edubits

Edubits commented Jul 21, 2015

Copy link
Copy Markdown
Contributor Author

Thanks in advance @wing328

wing328 added a commit that referenced this pull request Jul 21, 2015
[Swift] Support use in Framework
@wing328
wing328 merged commit 6b4f810 into swagger-api:develop_2.0 Jul 21, 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.

3 participants