-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
THRIFT-1681 #92
THRIFT-1681 #92
Conversation
Hey @djwatson, thanks for another patch, really appreciate your contributions back to Apache Thrift. looking at it quickly config check are good, compiler looks good, files added to dist (thanks, this often gets missed) and lib with test cases look good, if you could address the following I think this can get merged in
Thanks again |
Makefile.am builds C components - lua files are just copied as scripts. Not sure if this is correct? Test Plan: client/server test in thrift/test/lua Author: Andrew Birchall <[email protected]>
These should both be fixed - updated all the headers I found to apache (let me know if I missed any). Updated the TODO with description and remove name |
t_base_type::t_base tbase = ((t_base_type*)type)->get_base(); | ||
switch (tbase) { | ||
case t_base_type::TYPE_STRING: | ||
out << "'" << value->get_string() << "'"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
escaping?
@daurnimator could you provide a patch for this? thanks! |
Sorry @bufferoverflow, I have no need for this library any more. Just took a quick look at the diff as it passed through my inbox. |
Github Pull Request: This closes apache#92
Key space isolation extension to IDatabase
LUA thrift library merge from github.com/facebook/fbthrift
Makefile.am builds C components - lua files are just copied as scripts. Not sure if this is correct?
Test Plan:
client/server test in thrift/test/lua
Author: Andrew Birchall [email protected]