[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/thrift/pull/1156


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets

[dcelasun]

Github user dcelasun commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1156#discussion_r100089847
  
--- Diff: lib/go/test/tests/thrifttest_driver.go ---
@@ -162,7 +162,7 @@ func (p *ThriftTestDriver) Start() {
t.Fatal("TestStringMap failed")
}
 
-   setTestInput := map[int32]struct{}{1: {}, 2: {}, 3: {}}
+   setTestInput := []int32{1, 2, 3}
--- End diff --

Yeah, it's also finally serializable to JSON for non-primitive 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets

[jeking3]

Github user jeking3 commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1156#discussion_r100088653
  
--- Diff: lib/go/test/tests/thrifttest_driver.go ---
@@ -162,7 +162,7 @@ func (p *ThriftTestDriver) Start() {
t.Fatal("TestStringMap failed")
}
 
-   setTestInput := map[int32]struct{}{1: {}, 2: {}, 3: {}}
+   setTestInput := []int32{1, 2, 3}
--- End diff --

Syntax looks much better than before.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets

[jeking3]

Github user jeking3 commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1156#discussion_r97858435
  
--- Diff: lib/go/test/tests/thrifttest_handler.go ---
@@ -96,7 +96,7 @@ func (p *ThriftTestHandler) TestStringMap(thing 
map[string]string) (r map[string
return thing, nil
 }
 
-func (p *ThriftTestHandler) TestSet(thing map[int32]struct{}) (r 
map[int32]struct{}, err error) {
+func (p *ThriftTestHandler) TestSet(thing []int32) (r []int32, err error) {
--- End diff --

As long as it is a compile time failure if someone doesn't change it, 
that's good.  In C++ (C++03, or C++11 without the "override" keyword being 
used) if you change the signature of a virtual method, anything that overrides 
it will silently define a new method instead of overriding what you want.  It's 
pretty messy.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets

[dcelasun]

Github user dcelasun commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1156#discussion_r97843055
  
--- Diff: lib/go/test/tests/thrifttest_handler.go ---
@@ -96,7 +96,7 @@ func (p *ThriftTestHandler) TestStringMap(thing 
map[string]string) (r map[string
return thing, nil
 }
 
-func (p *ThriftTestHandler) TestSet(thing map[int32]struct{}) (r 
map[int32]struct{}, err error) {
+func (p *ThriftTestHandler) TestSet(thing []int32) (r []int32, err error) {
--- End diff --

Yes, this is a breaking change which was originally discussed in 
THRIFT-4011; I only started working on this PR after I was given the go ahead.

> How will this be communicated?

I would imagine posts to the mailing lists and an announcement on the 
website, several weeks in advance of a new release?

> What happens to existing handlers that implement the older method?

Compiling IDLs with this patch will change the signature of any RPC or 
struct in the generated Go code, so it's very easy to catch at compile time and 
make the changes.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets

[jeking3]

Github user jeking3 commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1156#discussion_r97841167
  
--- Diff: lib/go/test/tests/thrifttest_handler.go ---
@@ -96,7 +96,7 @@ func (p *ThriftTestHandler) TestStringMap(thing 
map[string]string) (r map[string
return thing, nil
 }
 
-func (p *ThriftTestHandler) TestSet(thing map[int32]struct{}) (r 
map[int32]struct{}, err error) {
+func (p *ThriftTestHandler) TestSet(thing []int32) (r []int32, err error) {
--- End diff --

This looks like a breaking change; any generated go implementation's 
handler will need to change.  How will this be communicated?  What happens to 
existing handlers that implement the older method?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets

[dcelasun]

GitHub user dcelasun opened a pull request:

https://github.com/apache/thrift/pull/1156

THRIFT-4011 Use slices for Thrift sets

As discussed in 
[THRIFT-4011](https://issues.apache.org/jira/browse/THRIFT-4011), this commit 
changes the Go generator to use slices, instead of maps for Thrift sets.

I've specifically didn't touch the Go library since there was no agreement 
on panicking for duplicates. We have three options:

1. Leave it as is and add documentation stating deduplication is the 
caller's responsibility.
2. Silently deduplicate before serialization.
3. panic on duplicates.

2 and 3 probably requires 
[`reflect.DeepEqual`](https://golang.org/pkg/reflect/#DeepEqual), which is not 
ideal.

@Jens-G Thoughts?

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dcelasun/thrift master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/thrift/pull/1156.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 #1156


commit 0e2e8c0b041300dafff641e19848a1e46df32bc6
Author: D. Can Celasun 
Date:   2017-01-15T09:53:19Z

THRIFT-4011 Use slices for Thrift sets

This commit changes the Go generator to use slices, instead of maps for 
Thrift sets.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---