You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by kdubb <gi...@git.apache.org> on 2015/07/04 04:40:25 UTC
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift co...
GitHub user kdubb opened a pull request:
https://github.com/apache/thrift/pull/539
Thrift 2905 & - Modern Objective-C & Swift compatibility.
This pull request is really just to start a conversation about how to move forward with the changes I've made to the Thrift Cocoa library and generator.
The nature of the changes are that they are big & a bit destructive. Unfortunately there wasn't much getting around that. Hopefully working with the community we can agree on the best way to integrate these changes. Maybe just as another "language" binding instead of replacing the current "cocoa".
## Goals ##
* Modernize generated code
* Increase asynchronous support (both blocks and promises)
* Remove use of deprecated classes & paradigms (e.g NSException, NSURLSession)
* Greater Swift interoperability
## Major Changes ##
* Use NSError based error & exception handling
* Optionally generate PromiseKit methods in the asynchronous clients
* Change asynchronous clients to be fully multi-thread safe
* Added THTTPSessionTransport, asynchronous HTTP transport that works with NSURLSession
* Use NS_ENUM enumerations (with standard format)
* Struct fields & their "set" flags now use properties exclusively
* Removed instance variables from public headers
* Remove retain/release stubs
* Remove all deallocs
# Why? #
We made some very large changes. Here's why...
## NSException to NSError ##
The use of NSException has been very problematic for us. Aside from the direction of Apple, which is to use exceptions for "fatal errors" only, it comes with a host of issues. First, Clang still produces incorrect code in many cases when using exceptions. Take this code for example...
```objective-c
executeBlockAndReturnValue(^{
@try {
return value;
}
@catch(NSException *e) {
}
// should be a return statement on this branch, Clang produces no error/warning
});
```
Clang should produce an error but doesn't. This is only a nuisance though, much worse are the ARC failures we've found when catching exceptions up the stack. Secondly, Swift has no support for exceptions (and with Swift 2 we've seen it won't ever use NSException).
It's for these reasons we changed the the Cocoa library to use NSError, all of the generated (synchronous) methods now follow the Apple guidelines and return a BOOL or object pointer with a last error parameter of type NSError**; return values of NO or nil mean the error parameter is populated and the call failed (or and exception was thrown from the server). These changes have been made throughout the generated and library code.
*Synchronous Client Method Mapping*
Only synchronous method mappings are affected. The asynchronous methods have ways to report errors out-of-band that don't require mapping types.
* Methods returning basic types are mapped to NSNumber/NSValue - nil reports call failure/exception
* Methods returning VOID are mapped to return BOOL - NO reports call failure/exception
* Methods returning Struct/String/Binary return the object value as normal - nil reports call failure/exception
### NSError & Swift 2 ###
These changes conveniently take the form of Swift 2's error handling system. This means that protocols and transports can be used from and written in Swift and will be compatible with the current & future directions of Swift.
## ARC Only ##
ARC is here to stay; Apple deprecated everything else a few years ago. All code related to maintaining backwards compatibly with non-ARC mode has been removed.
## Mutl-Threaded Asynchronous Transports ##
The current implementations means that clients cannot be used in different threads. This means the user is left to synchronize access to them or, as in our case, build a client per thread to ensure parallelism.
Our changes have reengineered the way asynchronous works. Now, asynchronous clients accept only protocol and async-transport factories. Each call allocates a new transport and protocol binds them before use. This provides the best options for parallelism with a fairly small overhead.
A note on performance... the alternative approaches, which we used in the past, required thread local storage inside the transport which meant an NSDictionary lookup (i.e. threadDictionary) for each read/write/flush. The new approach only requires 2 simple object allocations. Much improved.
## NSURLSession (NSURLConnection is now deprecated) ##
The new THTTPSessionTransport replaces the old THTTPClient. It's both asynchronous and uses the new NSURLSession classes for conformance in the new iOS/OSX versions.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/reTXT/thrift THRIFT-2905
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/539.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #539
----
commit a7e2544d55e366f1259afd508c3646cb64df716a
Author: Kevin Wooten <ke...@wooten.com>
Date: 2015-07-03T02:43:27Z
Modernized Cocoa
• Use NSError based error & exception handling
• Use NS_ENUM enumerations (with standard format)
• nullable & nonnull attributes for parameters
• Swift interoperability (nullability, enums & error handling)
• Removed instance variables from public header
• Remove retain/release stubs
• Remove all deallocs
commit e73f7b644cde831ed14729c0962a51d0476de788
Author: Kevin Wooten <ke...@wooten.com>
Date: 2015-07-03T18:32:19Z
Add optional support for PromiseKit to async clients
commit 1220b0a7d60d24680813dc214cd5464ebe0989b4
Author: Kevin Wooten <ke...@wooten.com>
Date: 2015-07-03T20:17:00Z
Fix analyzer surfaced issues with SSL socket
commit 0b91a7761c74e97783093885a92814f50aa03566
Author: Kevin Wooten <ke...@wooten.com>
Date: 2015-07-03T20:18:31Z
Protocols wrap transport errors with more information
The current message name & source file/line information is added to the protocol error that wraps the transport error.
commit 9925c463fd40466a8f2974869af01048767938a8
Author: Kevin Wooten <ke...@wooten.com>
Date: 2015-07-03T22:58:47Z
Make async transports & clients mutl-thread safe
commit 467efe8878357c48f3b2080c82a28d0bf86c7bd8
Author: Kevin Wooten <ke...@wooten.com>
Date: 2015-07-03T23:32:34Z
Rename transport "Clients" to transports. Client is the terminology for the top level object already.
commit fcc3ae3a47d0cbf4774b0ac8b7f543fe144db5cf
Author: Kevin Wooten <ke...@wooten.com>
Date: 2015-07-03T23:32:50Z
Fix async client init method
commit cb0f4325bef01ee9282a0055f5a29c415864fb9c
Author: Kevin Wooten <ke...@wooten.com>
Date: 2015-07-04T01:11:33Z
Fix setter name generation
commit 5ffe3478c01ee70e7489be196bc280aefe8ab5b0
Author: Kevin Wooten <ke...@wooten.com>
Date: 2015-07-04T01:11:55Z
Simplify NSError reading logic
commit 9a2bf0416d657ad643819aafa6943c1c8cc29ebc
Author: Kevin Wooten <ke...@wooten.com>
Date: 2015-07-04T01:12:51Z
Fix HTTP sessoin transport's response data loading
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146666197
I just found myself asking why we have ObjectiveC and Swift code liiving in one folder called Cocoa? I know the name already existed before, but does it make sense? Shouldn't it be two folders named ObjectiveC and Swift, respectively? Another case against the folder name Cocoa is that no other language folders are named after the underlying API.
Thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by jriskin <gi...@git.apache.org>.
Github user jriskin commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-148218066
Awesome! I'll check it out now. FYI, coercing the type solved my namespace issue. e.g.
for (id entry in self.feed.posts) {
if ([entry isKindOfClass:[CTZNPostModel class]]) {
// this next line creates a conflict on post_id since it's type is unknown at this point.
if([entry post_id] == self.current_post_id) {
// this fixes the problem...
if([(CTZNPostModel *)entry post_id] == self.current_post_id) {
Thanks for the quick fixes!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by creker <gi...@git.apache.org>.
Github user creker commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-147568526
You read my mind. Was thinking about both features you mention - somehow deal with conflicting names (shouldn't be too much of them) and store all value types in NSNumber. But latter will cause the fields to loose their type. Still, compiler options for both of them would be nice.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift co...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-120824710
@jimspeth These look like obvious changes. I will incorporate them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by jriskin <gi...@git.apache.org>.
Github user jriskin commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-147523571
First, thanks for all the modernization!
I had a few issues with this release.
1. had to delete the swift files
2. Using the new generator I had to remove instances of NSArray<SInt32> i just removed the typing for now
3. I'm having collisions with methods that I wouldn't suspect. For now i renamed all my properties, but shouldn't they some how be scoped to their class? I'm not entirely sure what's going on here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by jriskin <gi...@git.apache.org>.
Github user jriskin commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-147559312
I completely agree, definitely going to make the case to have the team defining the thrift to change... i would think it might be a good idea in the generator to automatically prefix reserved keywords as an option when the programmer can't control the thrift.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by creker <gi...@git.apache.org>.
Github user creker commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-147557593
Had a few problems with a field named `description`. It obviously conflicts with `NSObject`'s one. Have to be careful about method conflicts and, of course, don't use keywords as fields names. As far as I know, clang is smart enough to understand that `id` is used as a name and not a keyword. But I'm still against such names.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146952528
I've brought the exception reporting in line with other languages. I looked into Java & C++ to see the error codes they for all. Historically TApplicationError seems to have been correct in the past and I trampled on it by changing the codes themselves. The protocol & transport errors needed a bit of shuffling but now they are using a meaningful code in the correct context. I've kept some of the extended error information we were traditionally reporting but put it into variables that do not escape the current environment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by creker <gi...@git.apache.org>.
Github user creker commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-147547155
From the code it difficult to see what's the problem. Yes, in objc selectors are separate from the classes - they're shared between them and stored as plain strings. That creates problems only when you're dealing with `id` objects - compiler doesn't have type information. When you call method on `id` compiler will search for any class that contains the selector. But if your variable have a specific type then compiler will know what to do. Even with `id` I don't remember any conflicts - compiler just silently lets me call any selector that it can find which may crash at runtime because that particular object doesn't implement it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by jriskin <gi...@git.apache.org>.
Github user jriskin commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-147560463
It also might be nice to use NSNumber objects, would solve the issue of storing them in NSArrays.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by jriskin <gi...@git.apache.org>.
Github user jriskin commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-147554481
@creker ah, that makes some sense, I am using id for a couple of types of objects that I store in arrays and they don't resolve to a type until runtime. I'll revert all the code once i'm in a stable state and see if i can figure it out, for now i just renamed the conflicts.
Also, is it an issue that they are using 'id' as a variable name? That seems awfully unsafe in obj-c. I may push to see if i can get them to change this to post_id or something more reasonable.
e.g.
struct Post {
1: optional i32 id;
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146663420
Common mistake:
case t_base_type::TYPE_BYTE:
return "UInt8";
All Thrift integers are *signed*. The naming of the `byte` is a bit misleading.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-147854822
@jriskin PR #652 should fix your issues with ```NSArray<SInt32>```. They should now, correctly, be generated as ```NSArray<NSNumber *>```. It also fixes a number of other issues with constants of those types.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-147524306
@jriskin
Can you send me a sample of your thrift file that shows the issues with 2 & 3? Although, I can hazard a guess at 2.
Also, what was the reason for needing to delete the swift files? Are you targeting an platform version that doesn't support it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146665847
@Jens-G Should I change Objective-C as well? It was, and still is, treating ```byte``` as unsigned
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146669211
Will ensure the ASF header is atop all files.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift co...
Posted by jimspeth <gi...@git.apache.org>.
Github user jimspeth commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-120408184
I love it.
A couple of minor changes that will make the header files even more readable:
* Remove the `getter=` and `setter=` from the property declarations, since the format they use is the default already.
* Remove `- (instancetype) init;` from the generated interfaces, since that's defined by NSObject.
* Remove `read:` and `write:` methods from the interfaces, since they are declared by the `TBase` protocol.
* Move `validate:` to `TBase` protocol and remove from the interfaces.
Also, the `unset*` methods in the implementations are not used and not exposed in the interface. They could be removed from the implementation. A possible way to support unsetting values would be to have it generate overrides for the `*IsSet` properties, like this:
```
- (void) setMyValueIsSet: (BOOL) isSet {
_myValueIsSet = isSet;
if (!isSet) {
_myValue = nil;
}
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146394971
@Jens-G The Swift implementation is now fairly well tested. I used Objectice-C, Python and C++ to ensure it's compatibility.
Basically there's not much left to do with this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146440075
What about the compat breaks?
________________________________
Von: Kevin Wooten
Gesendet: 07.10.2015 22:11
An: apache/thrift
Cc: Jens Geyer
Betreff: Re: [thrift] Thrift 2905 & - Modern Objective-C & Swift Support (#539)
@Jens-G The entirety of the Objective-C changes are "complete" as far as I am concerned. We have been using them successfully for quite a while.
---
Reply to this email directly or view it on GitHub:
https://github.com/apache/thrift/pull/539#issuecomment-146314451
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146668825
There are some files (mostly *.swift) that do not have the ASF header on top.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-147544951
I assume from the way the IDL was pasted that all of the ```<i32>``` and ```<SInt32>``` have been scrubbed. Regardless, I believe I can see where the issue is with that.
> For now i renamed all my properties, but shouldn't they some how be scoped to their class? I'm not
> entirely sure what's going on here.
In Objective-C selectors are explicitly not scoped to their class. This has surprised me more times than I would like to believe.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift co...
Posted by creker <gi...@git.apache.org>.
Github user creker commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-145695896
When this will be merged? I'm really looking forward to this as it fixes almost everything that's wrong with cocoa library and compiler. It will break existing code but there is really no other way.
I don't think there's a point in keeping old implementation. Personally, I'd rather rewrite my code - it will make it cleaner and safer. If I want to stay on current implementation I will just keep using old library and compiler. It's been awhile since someone commited into cocoa and fixed some major problem any way so why not just left it as is and move on to the newer implementation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146668993
I had considered changing the generator name to "objc" for this exact reason. As I mentioned previously, the Swift generated code uses the same library implementation; which as I'm sure you're aware is written in Objective-C.
Until we decide (if ever) to have a dedicated Swift native library. I think the folder name is best left as cocoa which represents its shared nature between the Swift & Objective-C generators.
Also, I did add some Swift files to ease the code generation but even those build on the Objective-C implementations as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146670456
Re cocoa/objc/swift: I see. Leave it as it is.
Question: Are the eror codes from TTransportError.h (and mybe others) only used internally or are they visible to the other side? If they are public, they probably should match the usual Thrift error numbers, like in [this C++ code](https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/TTransportException.h), otherwise a (for example) C++ client will have a hard time dealing with them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146309057
Hi all,
first thank you for the contribution, we appreciate it. I have a few questions.
* Is everybody of you guys happy with it, or are there any loose ends that neec to be fixed?
* You wrote, it breaks compatibility. Can you explain that a bit more? Especially, how would people be affected that are using the curently existing solution? What do they need to do on their side when they decide to uograde Thrift to a version containing this stuff? I have to admit that I'm not really into that cocoa/swift stuff, so please bear with me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146446009
The biggest change is that simple types (bool, byte, etc) are now mapped differently to allow the proper use of NSError. These types now map to NSNumber instead. Aside from using NSError parameters that is the only material change.
e.g. this
NSError *error;
NSNumber *byte = client.testByte(&error);
UInt8 actualByte = byte.unsignedCharValue
if (!byte) {
//handle error
}
instead of...
@try {
UInt8 byte = client.testByte()
}
@catch(NSException *x) {
//handle error
}
Please understand that the current method (NSException) is severely deprecated and has been for years; even without Swift compatibility these changes should be made. The mapping changes I enacted are necessary to use the proper NSError based method.
> On Oct 8, 2015, at 12:10 AM, Jens Geyer <no...@github.com> wrote:
>
> What about the compat breaks?
> ________________________________
> Von: Kevin Wooten
> Gesendet: 07.10.2015 22:11
> An: apache/thrift
> Cc: Jens Geyer
> Betreff: Re: [thrift] Thrift 2905 & - Modern Objective-C & Swift Support (#539)
>
> @Jens-G The entirety of the Objective-C changes are "complete" as far as I am concerned. We have been using them successfully for quite a while.
>
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/apache/thrift/pull/539#issuecomment-146314451
> —
> Reply to this email directly or view it on GitHub <https://github.com/apache/thrift/pull/539#issuecomment-146440075>.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146314451
@Jens-G The entirety of the Objective-C changes are "complete" as far as I am concerned. We have been using them successfully for quite a while.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift co...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-145765106
@creker No Idea. I pinged them on the list a month or so ago. The response was favorable but no movement yet.
I just finished the rough pass of proper Swift code generation. It builds on these changes because it reuses the Objective-C implementations of protocols and transports. Hopefully this stuff will be interesting enough to get things moving.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by jriskin <gi...@git.apache.org>.
Github user jriskin commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-147543351
1. I believe it has something to do with Xcode converting the project to create a bridging header. If I drag all the files at once in to the project, it just accepts them, if you drag them one at a time it asks about creating the bridging header. It's probably harmless, but since I don't have any other swift code in the project, rather than set that up, I just removed the swift files.
https://developer.apple.com/library/ios/documentation/Swift/Conceptual/BuildingCocoaApps/MixandMatch.html
2. & 3 So push_id conflicts with a completely unrelated push_id method and post_ids are problem number 2.
thrift:
struct PushContentRequest {
1: optional i32 push_id;
2: optional list<i32> post_ids;
3: optional i32 post_limit;
4: optional i64 bookmark;
5: optional i32 comment_limit;
}
output header:
@interface CTZNAPIV2PushContentRequest : NSObject <TBase, NSCoding, NSCopying>
@property (assign, nonatomic) SInt32 push_id;
@property (assign, nonatomic) BOOL push_idIsSet;
- (void) unsetPush_id;
@property (strong, nonatomic) NSMutableArray<SInt32> * post_ids;
@property (assign, nonatomic) BOOL post_idsIsSet;
- (void) unsetPost_ids;
@property (assign, nonatomic) SInt32 post_limit;
@property (assign, nonatomic) BOOL post_limitIsSet;
- (void) unsetPost_limit;
@property (assign, nonatomic) SInt64 bookmark;
@property (assign, nonatomic) BOOL bookmarkIsSet;
- (void) unsetBookmark;
@property (assign, nonatomic) SInt32 comment_limit;
@property (assign, nonatomic) BOOL comment_limitIsSet;
- (void) unsetComment_limit;
- (instancetype) initWithPush_id: (SInt32) push_id post_ids: (NSArray<SInt32> *) post_ids post_limit: (SInt32) post_limit bookmark: (SInt64) bookmark comment_limit: (SInt32) comment_limit;
@end
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146666369
> Should I change Objective-C as well? It was, and still is, treating byte as unsigned
Sure. As I said, very common mistake.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146701420
With regard to the C++ and Cocoa exception codes matching... It doesn't seem like the exceptions are being thrown in the same circumstances in most cases. Maybe we should look at standardizing these codes in general?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-146937977
> Maybe we should look at standardizing these codes in general?
Good idea :-) In fact, the exception codes are already standardized for all standard Thrift exceptions (`TTransport/Protocol/ApplicationException`). If you compare with other languages you'll find they all match.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/539
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-147569380
FWIW, we currently use a property named "id". I'll admit to being concerned when we first started using it but beyond looking awkward it has produced no issues.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Thrift 2905 & - Modern Objective-C & Swift Su...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/539#issuecomment-147569087
The issue with the ```NSArray<SInt32>``` is just a problem with the generics specifier. It says SInt32 but all the code uses an NSNumber properly. I'll fix and commit ASAP.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---