[
https://issues.apache.org/jira/browse/THRIFT-242?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12662563#action_12662563
]
David Reiss commented on THRIFT-242:
------------------------------------
Hey, there is one thing that concerns me about this patch, regarding the
handling of container and structure fields with default values. For an
example, look at test/DebugProtoTest.thrift, the OneOfEach structure, the
i16_list field. The generated Python code looks like...
{code}
def __init__(self, im_true=None, im_false=None, a_bite=200, integer16=33000,
integer32=None, integer64=10000000000, double_precision=None,
some_characters=None, zomg_unicode=None, what_who=None, base64=None, byte_list=[
1,
2,
3,
], i16_list=[
1,
2,
3,
], i64_list=[
1,
2,
3,
],):
{code}
What this means is that all OneOfEachs constructed without specifying i16_list
will have a reference to the same list (Python default args are only evaluated
at definition time, not call time). So I wrote this test program...
{code}
#!/usr/bin/env python
import sys
sys.path.append('./gen-py')
from DebugProtoTest.ttypes import OneOfEach
ooe1 = OneOfEach()
ooe2 = OneOfEach()
print ooe2.byte_list[0]
ooe1.byte_list[0] = 0
print ooe2.byte_list[0]
{code}
And it prints 1, 0. I think this needs to be avoided. The two best solutions
I can come up with are...
1/ Default everything (at least non-scalars) to None, and make the body of the
__init__ say "if foo is None: foo = DEFAULT". The downside is that it becomes
impossible to override a default value with None (in the constructor).
2/ Take **kwargs. The downside is that it is less self-documenting. Also, I
think we would want to write code to throw an exception if someone specified an
invalid field.
Thoughts?
> Python struct constructors are clunky and error-prone
> -----------------------------------------------------
>
> Key: THRIFT-242
> URL: https://issues.apache.org/jira/browse/THRIFT-242
> Project: Thrift
> Issue Type: Improvement
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: init.patch, thrift-242_no_d_argument.patch
>
>
> The python constructors are clunky and unnecessarily difficult to use.
> Instead of
> Cls(value1, value2, ...)
> you must write
> Cls({'arg1': value1, 'arg2': value2})
> (or use the dict constructor instead of literal notation).
> Additionally, having the constructor silently do nothing if a non-dict is
> passed in (an object of the type being created would be another reasonable
> guess) or if an argument is mis-spelled causes users unnecessary trouble
> debugging.
> Since removing the d argument entirely would be backwards-incompatible, the
> attached patch keeps it, but puts real keyword parameters in for
> forwards-compatibility. Thus, old code will work with this patch, but new
> code can be written using keyword args that will still work when the d
> argument is removed in a release that allows backwards-incompatibility.
> (Removing the d argument is desireable because otherwise positional arguments
> can't be used.)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
