Skip to content

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

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

Open
iamsudip 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 all 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
21 changes: 19 additions & 2 deletions thriftpy2/thrift.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,26 @@ def __init__(self):
pass
return __init__

varnames, defaults = zip(*spec)
if hasattr(cls, 'thrift_spec'):
args = []
kwargs = []
varnames = []
defaults = []
for spec_element, t_spec in zip(spec, cls.thrift_spec.values()):
varnames.append(spec_element[0])
if t_spec[-1]:
args.append(spec_element[0])
else:
kwargs.append(spec_element)
defaults.append(spec_element[1])
Copy link
Copy Markdown
Contributor Author

@iamsudip iamsudip May 29, 2020

Choose a reason for hiding this comment

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

This is breaking lot's of valid tests

Copy link
Copy Markdown
Member

@ethe ethe May 29, 2020

Choose a reason for hiding this comment

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

I checked that it is all crashed in Client._send function, I think it is easy to be fixed.

Copy link
Copy Markdown
Contributor Author

@iamsudip iamsudip May 29, 2020

Choose a reason for hiding this comment

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

@ethe Can you tell me how can I achieve that?

I tried changing args_to_kwargs to not make args to kwargs, passed them to _send, but later failed at TProcessor:process_in:args = getattr(self._service, api + "_args")() because of required args and we removed them from defaults.

Copy link
Copy Markdown
Contributor Author

@iamsudip iamsudip May 29, 2020

Choose a reason for hiding this comment

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

Or we can add a fixture to be added to generated init for raising ValueError if required args don't have value.

Copy link
Copy Markdown
Member

@ethe ethe Jun 1, 2020

Choose a reason for hiding this comment

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

Compare with raising ValueError in runtime, I think it is better to use args because of a friendly function signature. I will fetch your branch and maybe research for a while. Thank you!

Copy link
Copy Markdown
Contributor Author

@iamsudip iamsudip Jun 1, 2020

Choose a reason for hiding this comment

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

@ethe Sure. I also think the same that using args is the way to go, I faced blocker here: Processor dependency on generated service api. Would love to learn from your implementation.

defaults = tuple(defaults) if defaults else None

args.extend(map('{0[0]}={0[1]!r}'.format, kwargs))
args = ', '.join(args)
else:
varnames, defaults = zip(*spec)
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