-
Notifications
You must be signed in to change notification settings - Fork 216
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
Add Swift refinements to several classes #241
Conversation
…oomState (and some for MXEvent)
…essionEventListener
…es to a couple MXRestClient methods
Running |
It passes validation for me. This is from a fresh clone of
Does |
Failures are not in the swift files themselves:
Do you use a .swift-version file? |
No, I don't use a .swift-version file. That error message looks familiar though. It's complaining about As a quick sanity check, could you clean the build folder from Xcode and try again? (option+shift+⌘+K, or hold option and select Product > Clean Build Folder) I think the last time this came up for me, I resolved it by changing the scope of the problem header from public to project. The current branch should have those headers scoped correctly, but you may have a stale build hanging around in your derived data directory (that has happened to me before too). |
Yes, I'm getting that error message too. I'm looking at this right now: http://stackoverflow.com/questions/32223965/xcode-7-code-coverage-data-generation-failed I'll let you know if I find a way to resolve it. |
I saw this SO post too but I did not want to add cocoapod scripts. |
@@ -0,0 +1,100 @@ | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright is not the same as in other files.
|
||
|
||
/* | ||
TODO: This method accepts a nil username. Maybe this should be called "anonymous registration"? Would it make sense to have a separate API for that case? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MXRestClient tries to be as close to the matrix CS API as possible. The username is optional in the CS API which is translated into a nullable parameter in this class.
|
||
|
||
/* | ||
TODO: Consider refactoring. The following three methods all contain (ruleId:, scope:, kind:). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The push rule API will be updated in short/mid terms. It does not worth to start a refactoring now.
`token` is an optional string, with nil meaning "initial sync". | ||
This could be expressed better as an enum: .initial or .fromToken(String) | ||
|
||
`presence` appears to support only two possible values: nil or "offline". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
presence is a string because the spec is open on possible values. Some use "unavailable" for example.
…generated a bridging header and made some automatic changes to the project file. This resolves the "Code Coverage Data Generation Failed" error.
The testing issue should be resolved now. By adding a swift file to the MatrixSDKTests bundle, Xcode automatically generated a Bridging Header file and changed some project settings. |
Sorry, it still does not work. I tried on 2 different macs. |
It definitely changed something in It can't be solely the workspace either. I can delete the workspace file, do a Anyways, I've tried several scenarios now, and it looks like simply having a swift file in the test bundle is enough to get Xcode and/or Cocoapods to run the tests consistently. |
Thanks. It works. |
Do you know a project with a pod that exposes both objc and swift api? My problem is that with this modified podspec file, when you do a I am wondering if it would be better to have 2 pods. MatrixSDK, the full objc pod as we have now and SwiftMatrixSDK, a copy of your MatrixSDK.podspec. What do you think? |
I'm not opposed to the idea of having a second pod that targets Swift specifically, but it should be possible to publish a pod that cleanly supports both languages. I can't think of any other projects that take a dual-pod approach like this. I'm going to fork riot-ios to see if I can get a feel for what the issue is. |
Okay, I was eventually able to get riot-ios to compile with the I believe all of the issues I encountered are solvable in a reasonable amount of time. My recommendation is to keep a single pod architecture. You could leave this PR open until the downstream issues are resolved, close it without merging, or pull it into a separate branch (called "Swift" or "Frameworks", or something). In the meantime, if users want to develop a client in Swift, they can reference my fork directly (or the "Swift" branch of this repository, if you choose to go that route). Read on for a summary of the issues I encountered. Google Analytics in MatrixSDKI think this was the most confusing one. Here's the error message:
I think this is caused by the Framework trying to initialize a class from a header file it doesn't own. The Google Analytics classes are available in the runtime (they're loaded in Riot), but they aren't linked directly to the framework. I got around this by using A better solution might be to create a GHMarkdownParser - MMIOT conflict(see also: matrix-org/matrix-ios-kit#188) The discount library used by GHMarkdownParser has two separate definitions of I don't know how to get around this one other than to remove GHMarkdownParser dependency from the MatrixKit podspec, and remove its usage from the project. There are other markdown parsers out there. You could probably find another one that will build as a Framework. Ambiguous Reference to ReadReceiptAlignmentThe compiler thinks that the Here's how to fix it: Some of the classes in riot-ios inherit from classes in MatrixKit (e.g. MXJingleCallStackThis is similar to the Google Analytics issue above. The MatrixSDK is depending on a framework it doesn't own (WebRTC). This could be resolved by moving the MXJingleCallStack out of MatrixSDK and either implement it in riot-ios directly (which actually imports Google WebRTC), or possibly spin it off into its own pod that clients can import if they desire. |
Thanks for this report. This is even worse than I saw at first. It looks like that swift wins over objc in a dual pod approach :/. I agree that we could get Riot build with this dual pod but Riot is not the only app using this sdk. The sdk was written in objc to make it usable in as many projects as possible. It looks like to me that having 2 pods could be a quick win:
|
Okay, you make a good point. The two pod approach is fine. However, the above issues will probably need to be addressed at some point. For example, the pod is currently incompatible with Google Analytics when I don't know much about publishing pods. How would you create a second pod from this repository? Is it just a matter of updating the existing podspec file with the second pod description? |
I would go with a 2nd podspec file called like SwiftMatrixSDK.podspec, which will be a copy of your current Matrix.podspec. You will probably need to change the pod name in the file but Cocoapods manages several podspec files in the same folder very well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A huge thanks for this awesome work!
This pull request adds Swift language refinements to the APIs in
MXRestClient
andMXSession
.NOTE: Swift requires iOS 8.0 or above. Therefore this PR breaks compatibility with iOS 7.0.
Summary:
kMXLoginFlowTypePassword
) are now available as Swift enums. Situations where a custom constant may be provided are handled with a.custom(String)
case (or similar).before
Issues
MXRestClient
initializer returns an optional.MXHTTPOperation
returned fromlogin()
. If the return value is unused, the compiler will throw a warning unless the programmer explicitly ignores it using the_ =
syntax.nil
)after
Improvements
MXRestClient
initializer no longer returns an optional@discardableresult
attribute, which permits the programmer to ignore the return value by default (the API still returns aHTTPOperation
, if it's desired)type:
is now an enum, enabling more concise usage. (In fact,.password
is the default value, so that parameter could be omitted altogether)Handling Callbacks in Swift:
MXResponse<T>
This enum enables Swift to return a single value that encapsulates both success and error conditions. The associated type for each case (either
T
orError
) is guaranteed to be non-nil.Usage:
Use a switch statement to handle both a success and an error:
Silently ignore the failure case:
Signed-off-by: Avery Pierce [email protected]