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

[Jens-G]

Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1156
  
Ok, thanks.


---
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 issue #1156: THRIFT-4011 Use slices for Thrift sets

[dcelasun]

Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1156
  
@Jens-G All tests green and rebased from master.


---
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 issue #1156: THRIFT-4011 Use slices for Thrift sets

[dcelasun]

Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1156
  
Whoops, I must have missed that one. Should be fixed now.


---
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 issue #1156: THRIFT-4011 Use slices for Thrift sets

[Jens-G]

Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1156
  
```
src/common/clientserver_test.go:61: cannot use handler (type 
*MockThriftTest) as type thrifttest.ThriftTest in argument to GetServerParams:
*MockThriftTest does not implement thrifttest.ThriftTest (wrong type 
for TestSet method)
have TestSet(map[int32]struct {}) (map[int32]struct {}, error)
want TestSet([]int32) ([]int32, error)
src/common/clientserver_test.go:226: cannot use s (type map[int32]struct 
{}) as type []int32 in argument to client.TestSet
FAILcommon [build failed]
make[2]: *** [check] Error 2
make[2]: Leaving directory `/thrift/src/test/go'
make[1]: *** [check-recursive] Error 1
make[1]: Leaving directory `/thrift/src/test'
make: *** [check-recursive] Error 1

```


---
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 issue #1156: THRIFT-4011 Use slices for Thrift sets

[dcelasun]

Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1156
  
@Jens-G Squashed commits and rebased from master.


---
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 issue #1156: THRIFT-4011 Use slices for Thrift sets

[dcelasun]

Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1156
  
I finally hade some time to take a look at this. Duplicate detection is now 
implemented with `reflect.DeepEqual`. It isn't really ideal, but Go lacks a way 
of defining equality for arbitrary types, so this is the only possible 
implementation that supports `set` for any `T`.


---
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 issue #1156: THRIFT-4011 Use slices for Thrift sets

[Jens-G]

Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1156
  
Ok, all fine. Don't panic ;-)


---
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 issue #1156: THRIFT-4011 Use slices for Thrift sets

[dcelasun]

Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1156
  
Hey @Jens-G sorry, I didn't have time to finish this up just yet. The lib 
part of the PR is still missing, but I'll be doing that in a few days. I'll 
rebase, push and let you know.


---
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 issue #1156: THRIFT-4011 Use slices for Thrift sets

[Jens-G]

Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1156
  
Hi @dcelasun,any news? In caseyou're donw with it, could you rebase the PR 
to trigger a new Travis build?


---
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 issue #1156: THRIFT-4011 Use slices for Thrift sets

[Jens-G]

Github user Jens-G commented on the issue:

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

Sent from mobile device, please ignore spelling mistakes.

Von: Duru Can Celasun
Gesendet: 16.01.2017 08:00
An: apache/thrift
Cc: Jens Geyer; Mention
Betreff: Re: [apache/thrift] THRIFT-4011 Use slices for Thrift sets (#1156)


Good intentions, absolutely, but the implementation ..

FWIW, I agree. However, we still need a solution and as I explained above, 
we don't have a way of returning an error here. That leaves us with silent 
deduplication (during serialization) in lib/go/thrift. Are you OK with that?

-
You are receiving this because you were mentioned.
Reply to this email directly, view it on 
GitHub, or 
mute the 
thread.



---
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 issue #1156: THRIFT-4011 Use slices for Thrift sets

[dcelasun]

Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1156
  
> Good intentions, absolutely, but the implementation ..

FWIW, I agree. However, we still need a solution and as I explained above, 
we don't have a way of returning an error here. That leaves us with silent 
deduplication (during serialization) in lib/go/thrift. Are you OK with that?


---
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 issue #1156: THRIFT-4011 Use slices for Thrift sets

[Jens-G]

Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1156
  
Oopsie. After I read the word "map" above I must have been mentally 
switched to maps somehow.  My mistake.

Nevertheless what I said about panic and errors in general above still 
holds true. At this time we have exactly 0 (zero) panics in the code under 
/lib/go/thrift. So I would argue that introducing a panic at least breaks the 
usual patterns of that library.

>  Speaking of which, panicking in case of programmer error is very common 
and idiomatic in Go. The standard library is full of such panics 

Put four Go experts together and you get five opinions, all idiomatic. 
Especially about the high and lofty Go error handling design principles:

> In Go, error handling is important. **The language's design and 
conventions encourage you to explicitly check for errors** where they occur (as 
distinct from the convention in other languages of throwing exceptions and 
sometimes catching them). 

Or mostly assigning them to `_` as many people do in Go. That's for the 
"encourage you to explicitly check for errors" part. In contrast, an exception 
must be caught, not only "sometimes", because there is no other error handling 
option. 

> The decision to not include exceptions in Go is an example of its 
**simplicity** and orthogonality. Using multiple return values and a simple 
convention, **Go solves the problem** of letting programmers know when things 
have gone wrong and **reserves panic for the truly exceptional**.

I can't see any "simplicity" in replacing exceptions by panics, 
"encouraging" error handling by leading people into misusing the `_` operator 
and at the same time claiming "*this time, we get it right*". Go's error 
handling is FUBAR from the beginning to the end. Good intentions, absolutely, 
but the implementation  All personal opinion, of course, 




---
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 issue #1156: THRIFT-4011 Use slices for Thrift sets

[dcelasun]

Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1156
  
Hey @Jens-G, I'm not sure I follow you, what does this have anything to do 
with maps? Assuming you meant sets, the docs say:

> An unordered set of unique elements. Translates to an STL set, Java 
HashSet, set in Python, etc. Note: PHP does not support sets, so it is treated 
similar to a List

Similar to PHP, Go does not have a native type for sets, so the best thing 
to do is to treat it similar to a list.

> Given that, I would say it could be one option to error, when the user 
inserts a duplicate.

This is not possible, because the caller doesn't "insert" anything, they 
simply return a slice. Consider the following:

```thrift
service Foo {
set bar() throws (1: Something error)
}
```

This generates an interface called `Foo` with the following method:

```go
type Foo interface {
Bar() ([]string, error)
}
```

So the user simply returns a string slice, the Thrift library has no 
control over it. Once `Foo` returns, it's too late for Thrift itself to return 
an error, only panic. Speaking of which, panicking in case of ***programmer 
error*** is very common and idiomatic in Go. The standard library is full of 
such panics (e.g search for "misuse" 
[here](https://golang.org/src/sync/waitgroup.go)). I would consider returning a 
non-unique slice for a Thrift set a programming error (and hence deserving a 
panic), but if you disagree, I can update the PR with deduplication in the 
library.


---
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 issue #1156: THRIFT-4011 Use slices for Thrift sets

[Jens-G]

Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1156
  
http://thrift.apache.org/docs/types states that (as one would expect) a 
Thrift map is defined as "A map of **strictly unique keys** to values. 
Translates to an STL map, Java HashMap, PHP associative array, Python/Ruby 
dictionary, etc.". 

Given that, I would say it could be one option to error, when the user 
inserts a duplicate. On the other hand, in that case the better option could be 
to simply replace the current value. 

But **what should not happen is that serialized data come in with 
duplicated keys**. That would be clearly an error as it is a violation of the 
rule above. In that case it might be absolutely ok to return some kind of error.

Re `panic()`, from my understanding of Go that's not the idiomatic way to 
go, since everything form tha above is an easily recoverable error.

Further reading:
 * https://blog.golang.org/error-handling-and-go
 * https://golang.org/doc/effective_go.html#panic
 * 
http://stackoverflow.com/questions/28472922/when-to-use-os-exit-and-panic-in-golang



---
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.
---