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

Disallow nesting apply functions #25

Closed
omus opened this issue Jul 5, 2017 · 6 comments
Closed

Disallow nesting apply functions #25

omus opened this issue Jul 5, 2017 · 6 comments

Comments

@omus
Copy link
Member

omus commented Jul 5, 2017

Nesting apply calls like:

apply(patch_compress) do
    apply(patch_file_paths) do

will result in only the last patch (patch_file_paths) being applied. Documentation needs to be improved regarding applying multiple patches along with throwing an exception if nested apply calls are detected.

@tpgillam
Copy link
Contributor

tpgillam commented Oct 1, 2021

I ran into this issue just now; my expectation is that nesting calls to apply should causes us to take the union of the current patch environment and the new patches. This is quite a convenient feature that I used quite a bit in python-land with unittest.mock.

Is there a reason that disallowing nesting is preferable to the union behaviour above?

@omus
Copy link
Member Author

omus commented Oct 1, 2021

Is there a reason that disallowing nesting is preferable to the union behaviour above?

The reason this is currently the case is that when applying patches they get set to a global variable which is then used by the @mock call sites (this is by far the weakest part of the package):

https://github.com/invenia/Mocking.jl/blob/a1a98dc3536075f86cc61aca8a591fce8dab0d84/src/patch.jl#L74-L76

We could use a stack of PatchEnv and combine them on-the-fly to get the effect you want but we need something better to address #89

@tpgillam
Copy link
Contributor

tpgillam commented Oct 1, 2021

I've started playing around in a draft PR here (I still would need to add documentation): #90

But indeed I hadn't thought about locking problems! My proposal was simply to use patches from the current PatchEnv when applying the new patches too.

@oxinabox
Copy link
Member

oxinabox commented Oct 1, 2021

We could use a stack of PatchEnv and combine them on-the-fly to get the effect you want but we need something better to address #89

This seems like an orthogonal problem.
Nested calls to apply seen no more (nor less) problematic to Task issues?

@omus
Copy link
Member Author

omus commented Oct 1, 2021

This seems like an orthogonal problem.
Nested calls to apply seen no more (nor less) problematic to Task issues?

I just brought it up as I expected whatever would fix #89 could also possibly fix this. But yes we can fix both of these issues separately

@tpgillam
Copy link
Contributor

tpgillam commented Oct 4, 2021

Closed by #90

@tpgillam tpgillam closed this as completed Oct 4, 2021
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

No branches or pull requests

3 participants