search

pinterest / riffed

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

Allow non-sequential arguments #29

Closed binaryseed closed 8 years ago

binaryseed commented 8 years ago

When the arguments to a service endpoint aren't strictly sequential, the current implementation breaks.

This PR adds in support for non-sequential arguments. My first commit demonstrates an example of non-sequential args, which causes the test to fail:

-  ActivityState setUserState(1: User user, 2: ActivityState status);
+  ActivityState setUserState(10: User user, 20: ActivityState status);

Instead of assuming sequential argument numbering, we simply pull the actual index from the metadata when calling build_arg_list

Thanks!!

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.03%) to 96.748% when pulling 538c3481c8a696933c6fc785da9f8ba578ab2385 on binaryseed:allow-non-sequential-arg-list into 4a2bbe184713b8c3369af1b62f50aeb84d349c4a on pinterest:master.

scohen commented 8 years ago

Interesting, can you add tests, or were the tests sufficient, since you just re-ordered the params used by the tests?

How did this come up? I don't think I've ever seen this before.

scohen commented 8 years ago

@binaryseed ^^

binaryseed commented 8 years ago

Thanks for reviewing!

The change I made to the server.thrift was sufficient to get us test coverage. Without my code change, the tests as I've modified them will fail.

I work at New Relic, and we have a ton of Java based services. Our Java devs like to number their arguments 10, 20, 30 etc, so that if they want to insert args in between any other args in the future, they can do so w/o breaking backward compatibility, since it's the number of the arg that identifies it, not just it's order in the list. They'd just add an optional arg numbered 15 for example.

All of the thrift files in the tests, and presumably in your services, probably use simple 1, 2, 3 sequential ordering, which is why this never would have come up :)

Thanks for open sourcing this, by the way!

scohen commented 8 years ago

Fascinating, @binaryseed. That seems weird though; if they're adding arguments that are optional, why bother with the sequence numbers? Is the ordering important to them?

All of the thrift files in the tests, and presumably in your services, probably use simple 1, 2, 3 sequential ordering, which is why this never would have come up :)

Yes, exactly.

binaryseed commented 8 years ago

Yeah, I can't claim the reasoning is sound, but the implementation here now handles all cases! Thanks!