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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added tests to exercise intercepting an async method #429

Closed
wants to merge 6 commits into from

Conversation

JSkimming
Copy link
Contributor

@JSkimming JSkimming commented Jan 16, 2019

I'm re-raising this PR originally raised as #337 to present the problem.

I'm going to then merge the changes from @brunoblank in #428 and re-factor given the breaking changes to see if it fixes the issue (spoiler alert, it does on my machine 馃槂)

@JSkimming
Copy link
Contributor Author

JSkimming commented Jan 16, 2019

As shown by the passing builds, the changes introduced by PR #428 address the problem I outlined in issue #145 specifically this comment.

@stakx
Copy link
Member

stakx commented Mar 11, 2019

@JSkimming - Apologies for the long silence from the Castle team. I've been wanting to take a careful look at your PR and the associated issue for a long time, but have been very preoccupied with work. This is just a small ping to show that this hasn't been forgotten. I'm planning to get back to work here at this repo soon... but it might take me another few weeks.

@stakx
Copy link
Member

stakx commented Mar 21, 2019

@JSkimming - Thank you for adding a unit test to demonstrate a problem that #428 would solve. 馃憤

However we now have two PRs that propose the same basic change to DynamicProxy's API; I've posted a review comment over in #428 that would also apply here. In order to not spread discussion over too many separate pages and end up with excessive cross-referencing, I propose that we discuss this change over in #428 or, if we stray too far from it, go back to the issue (#145). Is this OK with you?

@JSkimming
Copy link
Contributor Author

@stakx of course makes perfect sense.

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.

None yet

2 participants