search

pinterest / riffed

Provides idiomatic Elixir bindings for Apache Thrift
Apache License 2.0
308 stars 37 forks source link

Add exceptions #20

Open scohen opened 8 years ago

scohen commented 8 years ago

This PR adds exceptions to the client, server and struct modules. It's dependent on thrift 0.10 though, and targets the 1.5 branch.

scohen commented 8 years ago

@laozhp @tbug How is this?

jparise commented 8 years ago

Maybe this should require Thrift 0.10 in mix.exs (via thrift_version).

The .travis.yml script will also need to an update to use Thrift 0.10 in the CI environment.

laozhp commented 8 years ago

Good job! I had found the generated struct_names() not include exception names, so I add a PR(https://github.com/apache/thrift/pull/975) to thrift compiler to generate exception_names() meta info.

scohen commented 8 years ago

@jparise I can't, it's not out yet.

@laozhp Thanks for putting that fix in; I'm sad to say that I omitted exceptions when I made the original change. Once this lands, I can just ask for the exceptions rather than relying on them not being in the struct list.

tudborg commented 8 years ago

Sorry for not getting back sooner. I'll take a look at this asap, but at first glance it looks great 👍

tudborg commented 8 years ago

Ah, sorry for not getting back. I'm having trouble building thrift 0.10, havn't had time to debug the issue yet.

scohen commented 8 years ago

@tbug apparently, so is apache ;)

To say we're disappointed with the speed of thrift releases is an understatement.

tudborg commented 8 years ago

@scohen Ha, alright. I guess I'm happy it's not just me then 😄

I'm not really following thrift dev, so I have no idea how far we are from 0.10, but I'd expect it to be around the corner? Out of curiosity, which parts of this PR depends on 0.10?

scohen commented 8 years ago

@tbug As far as I can tell from the outside, they've tagged the release and are now doing... something.

This PR relies on deeper metadata around functions and exceptions, which were added to 0.10.

scohen commented 8 years ago

@binaryseed Sorry it's been so long, but I updated exception handling on the v1_5_0 branch. Please give it a shot. I also made full end-to-end unit tests so that I can be sure that both the server and client handle exceptions properly.