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

[iamsudip]

No description provided.

[codecov]

Codecov Report

Merging #132 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
+ Coverage   83.09%   83.13%   +0.03%     
==========================================
  Files          43       43              
  Lines        3911     3920       +9     
==========================================
+ Hits         3250     3259       +9     
  Misses        661      661              

Impacted Files	Coverage Δ
thriftpy2/thrift.py	89.23% <100.00%> (+0.34%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b981e8c...5617a55. Read the comment docs.

[iamsudip]

@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?

[iamsudip]

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?

[ethe]

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

[iamsudip]

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.

[iamsudip]

@ethe Look at this commit: 5617a55

[iamsudip]

This is breaking lot's of valid tests

[ethe]

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

[iamsudip]

@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.

[iamsudip]

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

[ethe]

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!

[iamsudip]

@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.