Skip to content

Convert function, method, builtin_*, frame, and generator to Any payload #642

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.

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

Merged
merged 5 commits into from
Mar 10, 2019

Conversation

OddCoincidence
Copy link
Contributor

@OddCoincidence OddCoincidence commented Mar 9, 2019

Getting so close now!

@codecov-io
Copy link

codecov-io commented Mar 9, 2019

Codecov Report

Merging #642 into master will decrease coverage by 0.02%.
The diff coverage is 27.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #642      +/-   ##
==========================================
- Coverage   40.81%   40.78%   -0.03%     
==========================================
  Files          76       77       +1     
  Lines       17345    17383      +38     
  Branches     4474     4484      +10     
==========================================
+ Hits         7079     7090      +11     
- Misses       8366     8396      +30     
+ Partials     1900     1897       -3
Impacted Files Coverage Δ
vm/src/frame.rs 47.4% <10%> (+0.98%) ⬆️
vm/src/obj/objframe.rs 30.23% <100%> (-0.72%) ⬇️
vm/src/obj/objbuiltinfunc.rs 33.33% <33.33%> (ø)
vm/src/vm.rs 54.12% <66.66%> (-0.33%) ⬇️
vm/src/obj/objgenerator.rs 40.57% <7.14%> (-3.87%) ⬇️
vm/src/obj/objfunction.rs 32.94% <7.69%> (-5.68%) ⬇️
vm/src/pyobject.rs 61.52% <75%> (+0.59%) ⬆️
... and 4 more

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 010969f...6eea407. Read the comment docs.

@OddCoincidence OddCoincidence changed the title Convert function / method to Any payload Convert function, method, builtin_*, frame, and generator to Any payload Mar 9, 2019
trace!("invoke __call__ for: {:?}", payload);
self.call_method(&func_ref, "__call__", args)
}
if let Some(PyFunction {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is a match statement no option here? I feel match is better than if statements with returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can get rid of the explicit returns, but a match won't work here because each payload call is generic over the specific payload type.

(I do have some ideas on how to make this kind of thing cleaner and more efficient, but they're not fully baked yet.)

Copy link
Member

Choose a reason for hiding this comment

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

I think we could use a macro, something like match_payload!, which would look like:

let match_res = match_payload!(pyobjref, {
    objint::PyInt(int) => do_thing(int.value),
    objfloat::PyFloat(objfloat::PyFloat { value }) => do_float_thing(value),
});

@windelbouwman
Copy link
Contributor

I had some minor comment, but it looks good!

@windelbouwman windelbouwman merged commit 79b4f62 into master Mar 10, 2019
@OddCoincidence OddCoincidence deleted the joey/function-to-any branch April 9, 2019 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants