Skip to content
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.

Sign up for GitHub

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

[WIP] thrift: treat required fields as positional arguments while generating init, closes #119 #132

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+20 −3
Open

[WIP] thrift: treat required fields as positional arguments while generating init, closes #119 #132

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
094b747
thrift: treat required fields as positional arguments while generatin…
iamsudip May 28, 2020
8d2e8e1
tests: add noqa to suppress E402
iamsudip May 28, 2020
5617a55
thrift: remove defaults for required arguments in generated init
iamsudip May 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/test_http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
thriftpy2.install_import_hook() # noqa

from thriftpy2.http import make_server, make_client, client_context # noqa
from thriftpy2.thrift import TApplicationException
from thriftpy2.thrift import TApplicationException # noqa


addressbook = thriftpy2.load(os.path.join(os.path.dirname(__file__),
Expand Down
15 changes: 14 additions & 1 deletion thriftpy2/thrift.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,22 @@ def __init__(self):
pass
return __init__


iamsudip marked this conversation as resolved.
Show resolved Hide resolved
varnames, defaults = zip(*spec)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethe This function still returning the same function as before, just that init generated would have positional arguments now cause I didn't change defaults variable which is being passed to types.FunctionType while returning. What do you think?

Copy link
Contributor Author

@iamsudip iamsudip May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we change defaults, I don't think this commit makes any difference.

base.thrift:

struct Hello {
    1: optional string name,
    2: required string greet
}

In [4]: ab = thriftpy2.load("tests/base.thrift")

In [6]: ab.Hello.__init__??
Signature: ab.Hello.__init__(self, greet=None, name=None)
Source:   
def __init__(self, greet, name=None):
    self.name = name
File:      ~/Work/Personal/thriftpy2/<generated Hello.__init__>
Type:      instancemethod

In [9]: ab.Hello()
Out[9]: Hello(name=None, greet=None)

This should raise error, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think raising error here should be better, maybe we can remove required arguments from defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing required arguments from defaults breaking lots of tests. Look at new change and breaking test cases. I guess we can keep the signature intact and add ValueError statements inside init if args are None and if required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethe Look at this commit: 5617a55

if hasattr(cls, 'thrift_spec'):
args = []
kwargs = []
for spec_element, t_spec in zip(spec, cls.thrift_spec.values()):
if t_spec[-1]:
args.append(spec_element[0])
else:
kwargs.append(spec_element)

args.extend(map('{0[0]}={0[1]!r}'.format, kwargs))
args = ', '.join(args)
else:
args = ', '.join(map('{0[0]}={0[1]!r}'.format, spec))

args = ', '.join(map('{0[0]}={0[1]!r}'.format, spec))
init = "def __init__(self, {}):\n".format(args)
init += "\n".join(map(' self.{0} = {0}'.format, varnames))

Expand Down