You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by zhaakhi <gi...@git.apache.org> on 2016/05/02 18:01:51 UTC
[GitHub] thrift pull request: THRIFT-3812 Add option to prefix namespace Sw...
GitHub user zhaakhi opened a pull request:
https://github.com/apache/thrift/pull/1002
THRIFT-3812 Add option to prefix namespace Swift type names
Swift does not appear to have a way to namespace things within a single pod.
This adds an option swift:prefix_namespace that will add Cocoa-style prefixes to the generated code if the file has a defined Swift namespace.
E.g. if a file has
"namespace swift ABC"
then the struct "MyStruct" will be renamed to "ABCMyStruct"
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zhaakhi/thrift THRIFT-3812
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1002.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 #1002
----
commit a7239140a64ae95ddf06d057070b4bbde2bc7895
Author: H�kon Hitland <ha...@zedge.net>
Date: 2016-04-20T19:59:42Z
THRIFT-3812 Add option to prefix namespace Swift type names
Swift does not appear to have a way to namespace things within a single
pod.
This adds an option swift:prefix_namespace that will add Cocoa-style
prefixes to the generated code if the file has a defined Swift
namespace.
E.g. if a file has
"namespace swift ABC"
then the struct "MyStruct" will be renamed to "ABCMyStruct"
----
---
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 issue #1002: THRIFT-3812 Add option to prefix namespace Swift type na...
Posted by zhaakhi <gi...@git.apache.org>.
Github user zhaakhi commented on the issue:
https://github.com/apache/thrift/pull/1002
Hello, sorry that I haven't had time to work more on this.
In principle it could be merged with just a rebase and a change to the option name.
In practice https://github.com/apache/thrift/pull/1084 should take priority, and may reduce the need for this by using module namespacing.
If anyone still wants this after https://github.com/apache/thrift/pull/1084 has been merged, I could do a rebase and fix it up.
---
[GitHub] thrift issue #1002: THRIFT-3812 Add option to prefix namespace Swift type na...
Posted by apocolipse <gi...@git.apache.org>.
Github user apocolipse commented on the issue:
https://github.com/apache/thrift/pull/1002
Yeah I think that would be the most appropriate way, making sure that people that use it /want/ to use it and/or know that its not a Swift convention.
SPM integration should come with Swift 3.0 support, which while in beta now is still not complete.
Sidenote: If anyone's interested I'm working on building a Pure-Swift Thrift Library, basing it off of Swift 3.0 now (Started with Swift 2.0, but it was limited to Mac/iOS environments due to Foundation use, now Swift Foundation is part of Swift Standard Library 3.0 can be pushed for a public lib that works on Linux + Mac) It's going to be in the works until Swift dev's finish up Foundation (still a bit incomplete as of writing this) but it will be much more "Swifty" (Following Swift 3.0 API Guidelines quite strictly), and may require an updated Code generator as well. I'm not too familiar with the Code generators so should someone like to work with me for a Swift 3.0 Library + Code Generator that'd be great :D (Ideally i'm shooting to make it still compatible with the existing Swift generator, but should any changes be needed we should add a 3.0 generator option)
---
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 issue #1002: THRIFT-3812 Add option to prefix namespace Swift type na...
Posted by mfrawley <gi...@git.apache.org>.
Github user mfrawley commented on the issue:
https://github.com/apache/thrift/pull/1002
@apocolipse Like the ideas. Protocols can't be nested under a struct. You get:
```
error: declaration is only valid at file scope protocol MyService {}
```
I like the Swift packages suggestion (which unfortunately are not in a stable Swift release yet). Frameworks influence app startup time and Cocoapods/Carthage already provide this functionality with podspecs (we use Cocoapods atm).
A bigger issue we've run into when updating (since we generate code for 6+ client langs) is that we have property naming collisions .e.g. .code in subtypes of NSError had to be renamed to errorCode, or .description. Being able to have some way of dealing with that would be nice. One possible solution might be a language-specific alias e.g. generate 'errorCode' for Swift, but leave it as 'code' for other langs. That would avoid having to uglify all client code with manually prefixed names. It's a different issue I know but it's also name-collision related, and has cost me some considerable effort.
---
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 issue #1002: THRIFT-3812 Add option to prefix namespace Swift type na...
Posted by apocolipse <gi...@git.apache.org>.
Github user apocolipse commented on the issue:
https://github.com/apache/thrift/pull/1002
In my opinion this is very Un-Swifty. Namespace prefixes are an Objective-C Cocoa convention, Swift tries to get away from this convention (in fact with Swift 3.0, NS prefixes on many Foundation types have been dropped*)
It would be more proper to Nest them instead of rename them, and not that difficult either, simply install everything in a struct:
```
namespace swift MyNamespace
struct MyStruct { ... }
service MyService { ... }
```
```swift
struct MyNamespace {
struct MyStruct { ... }
protocol MyService { ... }
}
```
Alternatively, Swift uses Module scoping for namespacing, so having the generator spit out an Xcodeproj with a Framework scheme, or SPM formatted file hierarchy, would be ideal, this would give you import semantics and direct access to your types when there's no conflicts,
```swift
import MyNamespace
var aStruct = MyStruct()
```
as well as nested access when conflicts exist
```swift
var aStruct = MyNamespace.MyStruct()
```
Either module scoping or nesting are the ideal Swifty ways to leverage namespaces, Name prefixing shouldn't be used here.
*Dropped where types have been added as value types for more Swift-like handling, mutability, etc.
---
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 issue #1002: THRIFT-3812 Add option to prefix namespace Swift type na...
Posted by zhaakhi <gi...@git.apache.org>.
Github user zhaakhi commented on the issue:
https://github.com/apache/thrift/pull/1002
Proper Swift modules would be great. The Cocoa-style prefixes have worked well for us as a stop-gap solution, though.
I don't mind changing the option name or description if that would clarify things.
---
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 issue #1002: THRIFT-3812 Add option to prefix namespace Swift type na...
Posted by apocolipse <gi...@git.apache.org>.
Github user apocolipse commented on the issue:
https://github.com/apache/thrift/pull/1002
So for Cocoapods/Carthage, you're also using Frameworks (startup time is negligible). I'd almost recommend manually handling separate Podspec's to keep things Module scope namespaced, it doesn't seem right at this point to have Thrift generate Carthage/Cocoapods configurations. SPM should be good enough when Swift 3.0 is stable to where it can point to specific files for modules, additionally Foundation is ready for server side use then as well so perhaps SPM is the way to go.
Either way I wouldn't recommend this as a 1st-class feature for Swift code generation from Thrift. Perhaps as an option "Use Obj-C Cocoa Namespacing", so that its explicitly clear that this isn't a Swift convention, but an old Obj-C/Cocoa convention.
---
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 issue #1002: THRIFT-3812 Add option to prefix namespace Swift type na...
Posted by mfrawley <gi...@git.apache.org>.
Github user mfrawley commented on the issue:
https://github.com/apache/thrift/pull/1002
This is a must-have for Swift support. Any eta on merging?
---
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 issue #1002: THRIFT-3812 Add option to prefix namespace Swift type na...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1002
This hasn't been touched in over a year - what needs to be done?
---