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.

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

Keyword arguments brain dump #7

Open
christopher-dG opened this issue Oct 9, 2019 · 1 comment
Open

Keyword arguments brain dump #7

christopher-dG opened this issue Oct 9, 2019 · 1 comment
Labels
Function: mock Problems with the mock function.

Comments

@christopher-dG
Copy link
Member

christopher-dG commented Oct 9, 2019

Here is all my thoughts about trying to support keyword arguments so that I don't go down the same rabbit hole more than once.

Cassette doesn't really support overdubbing functions with keyword arguments right now but you can sort of do it with JuliaLabs/Cassette.jl#48 (comment).

Idea 1: Implement that overdub method with the kwftype along with each normal overdub implementation
Problem: How to recurse with keywords? That seems completely unsupported right now. Trying recurse(ctx, () -> f(args...; kwargs...) just loops back around forever.

Solution?: Get the body method that contains all the keywords as positional arguments with LoweredCodeUtils, then call that method with the keywords
Problem: There are no longer any default values so this dies if the user didn't supply all keywords.
Solution?: Find the default values by reading the source code (functionloc, Meta.parse, etc.)
Problem: This gets us into eval hell, and we can't possibly cover all the ways that the functions are defined (via macros, generated functions, etc.)

Solution?: Add one method to each mocked function that takes a new Keywords struct (owned by SimpleMock to avoid piracy), then recurse into that instead

Problem: I haven't tried to implement this but I think this gets us into the same loop as () -> f(args...; kwargs...) (where the recurse just ends up calling the function with keywords again, hitting the overdub and recursing again)

Solution?: Go back to multiple dynamically-generated Context types, and only support keywords when there are no filter functions (should_mock always returns true, so omit it). This is the only one that might actually work, which kind of sucks. But i probably shouldn't work too hard for this when Cassette should in theory eventually support kwargs by itself.
Problem: Goodbye speed

Actual solution: Learn how Cassette and/or Julia work and help get kwargs supported in Cassette

One more thing to note: Choosing whether or not to implement overdub for keywords per-function is not helpful, because we want to be able to mock calls to non-kwfuncs even if they have keywords.

christopher-dG added a commit that referenced this issue Oct 9, 2019
This implements the last solution outlined in #7.
For some reason, it's a lot faster than I remember this approach
being.

TODO:
- update docs to include section about reusing contexts
- add note in docs to explain when kwargs are discarded
- add tests for keyword arguments, discarding, and warning
@christopher-dG
Copy link
Member Author

The last solution (multiple context types, keywords are dropped on recurse) is implemented and I'll probably release that. Keeping this open until Cassette supports kwargs for real though.

christopher-dG added a commit that referenced this issue Oct 10, 2019
This implements the last solution outlined in #7, and drops keywords
from the call when a recurse happens.
For some reason, it's faster than I remember this approach being.
@christopher-dG christopher-dG added the Function: mock Problems with the mock function. label Jan 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Function: mock Problems with the mock function.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant