[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1461 The reasoning for this is that Apache maintains its own master repository; so pull requests here have to be pulled into the master by a committer, and then the github mirror updates. If the commit message contains a "This closes" or "This fixes" line item then it closes the github issue or pull request when that occurs. I'll merge this tonight - thanks. ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 > For Thrift, PRs are not merged, just closed. You'll see a commit with you as the author but someone else as the committer, like these. Ah got it, good to know thanks! ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 > GitHub can now automatically squash commits in PRs at merge time For Thrift, PRs are not merged, just closed. You'll see a commit with you as the author but someone else as the committer, like [these](https://github.com/apache/thrift/commits/master). ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 Travis was green so i've squashed it! FYI for the future: GitHub can now automatically squash commits in PRs at merge time. If that feature is enabled for the repo it shouldn't be necessary for committers to squash their branch prior to merging, since everything can be squashed via the GitHub UI:  ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 Happy to help. Once Travis is all green, please squash your commits so it'll be easier to merge. ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 Done ^. Thanks again for all your direction Dcelasun ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 Exactly. ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1461
Good call, so then:
```
func (p *BaseClient) Client_() thrift.TClient {
return p.c
}
```
?
---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1461
Any capitalized method on the client will potentially conflict with an RPC,
e.g:
```thrift
service Foo {
string C()
}
```
will generate uncompilable code. I suggest we use something like `Client_`
as the method name. It looks ugly, but there is precedent for it (conflicts
with keywords etc).
---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1461
This is a lot cleaner -- I like it. We still have the same issue with
tests, though, if the base service is in a separate package -- you can't access
the client as `.c` on the base service since it's lowercased.
Maybe we expose the client with a getter method?
```
func (p *BaseClient) C() thrift.TClient {
return p.c
}
```
---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 > So basically the logic would be to only add `c: thrift.TClient` to the `*Client` struct if `extends.empty()` in the generator? Exactly. ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 You're correct that it's a service extending another service; tbh I missed that too (I'm not the author of the `.thrift` files. I'm also generally new to thrift so still sorting out what should be generated). So basically the logic would be to only add `c: thrift.TClient` to the `*Client` struct if `extends.empty()` in the generator? I'll take a look and see how hard that looks. ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 Ah, you have a service extending another service. I missed that. In that case, I think removing `c thrift.TClient` from `UserServiceClient` (and constructor functions) is the cleanest option. That way, we would only have a `TClient` within the `UserServiceBase` and it can be reused by any extending service. Thoughts? ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1461
> Are you getting this second "Base" struct with 0.11 compiler? Because I
can't reproduce.
I've been working off the master branch, but I just tried generation with
0.11 (327ebb6c2b6df8bf075da02ef45a2a034e9b79ba) and it looks like it has the
same issue. Here is a direct copy-paste from my generated code with 0.11:
```
// Deprecated: Use NewUserService instead
func NewUserServiceClientFactory(t thrift.TTransport, f
thrift.TProtocolFactory) *UserServiceClient {
return {UserServiceBaseClient:
NewUserServiceBaseClientFactory(t, f)}
}
// Deprecated: Use NewUserService instead
func NewUserServiceClientProtocol(t thrift.TTransport, iprot
thrift.TProtocol, oprot thrift.TProtocol) *UserServiceClient {
return {UserServiceBaseClient:
NewUserServiceBaseClientProtocol(t, iprot, oprot)}
}
func NewUserServiceClient(c thrift.TClient) *UserServiceClient {
return {
c: c,
UserServiceBaseClient: NewUserServiceBaseClient(c),
}
}
```
---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1461
```go
type MyServiceClient struct {
c thrift.TClient
*MyServiceBaseClient
}
type MyServiceBaseClient struct {
c thrift.TClient
}
```
@johnboiles Are you getting this second "Base" struct with 0.11 compiler?
Because I can't reproduce.
> Also should I remove the // Deprecated comments if these are not actually
deprecated?
Yes please.
---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 @dcelasun any thoughts on fixing the tests? Looks like `tutorial/go/src/tutorial/tutorial.go` imports its `BaseClient` from another package. Since `c` isn't public in that other package, my modified initializer fails. Probably the simplest fix would just be to add a public accessor for `c`, (e.g. `func (p *BaseClient) C() thrift.TClient`) and us that public access in my modified initializer methods. What do you think? ---
