Support nested definitions and fields with capitals #5
Conversation
indygreg
left a comment
indygreg
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
I haven't touched lua-protobuf in years, so it is an effort for me to remember how this all works. And when I say "this all" I literally mean both Lua and Protocol Buffers, as I haven't used either in years.
I found some style issues with your PR but haven't really fully digested the content changes yet because I need to find time to page this back into my brain. So consider this a partial review.
If anyone else is watching and wants to help with the meat of the review, I'd appreciate the help! Be warned: if you sound too competent I may ask you to become a project maintainer ;)
| return '%s%s_' % (package_function_prefix(package), message) | ||
|
|
||
| def message_open_function_name(package, message): | ||
| def message_open_function_name(package, message, namespace=[]): |
There was a problem hiding this comment.
Using [] as a default value is one of the most common Python gotchas. See http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments for more.
The correct way to write this is:
def func(namespace=None):
namespace = namespace or []
or
def func(namespace=None):
if namespace is None:
namespace = []
| lines.append('string s = m->%s();' % name) | ||
| lines.append('m->has_%s() ? lua_pushlstring(L, s.c_str(), s.size()) : lua_pushnil(L);' % name) | ||
| lines.append('string s = m->%s();' % name.lower()) | ||
| lines.append('if (m->has_%s()) lua_pushlstring(L, s.c_str(), s.size()); else lua_pushnil(L);' % name.lower()) |
There was a problem hiding this comment.
Nit: Please don't make unrelated changed to code structure in the same commit as a substantive change, as this makes it more difficult to perform code review and to bisect failures later. Please revert back to the ternary operator or move this to its own commit.
Also, is if (expr) statement; else statement; valid syntax?! I don't think I've ever seen that. I don't like blocks without { ... } because of things like the infamous "goto fail" bug. So if you are going to change this, please use common syntax.
| ] | ||
|
|
||
| def message_method_array(package, descriptor): | ||
| def message_method_array(package, descriptor, namespace=[]): |
There was a problem hiding this comment.
Another instance of a mutable default argument.
| return lines | ||
|
|
||
| def message_header(package, message_descriptor): | ||
| def message_header(package, message_descriptor, namespace=[]): |
There was a problem hiding this comment.
And another. (This will be the last time I mention it.)
|
|
||
| for descriptor in file_descriptor.message_type: | ||
| lines.extend(message_header(package, descriptor)) | ||
| lines += message_header_recursive(package, descriptor) |
There was a problem hiding this comment.
Please change this back to lines.extend(), as I have a personal bias against the += operator because it often doesn't do what you intended and can mask bugs.
|
|
||
| nested_namespace = namespace + [descriptor.name] | ||
| for nested in descriptor.nested_type: | ||
| lines += enum_source_recursive(nested, nested_namespace) |
2 participants
This PR expands lua-protobuf to support: