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

Constructor replacement #16

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

manne
Copy link

@manne manne commented Jan 28, 2018

adds functionality to replace the constructor invocation with the constructor from a different class

@tonerdo
Copy link
Owner

tonerdo commented Jan 30, 2018

Thanks for the contrib @manne. Will take a look later today

@tonerdo
Copy link
Owner

tonerdo commented Mar 28, 2018

I haven't forgotten this. Just holding of on merging it until I get a chance to test it out. Currently preoccupied with some bug fixes

@manne
Copy link
Author

manne commented Jun 17, 2018

@tonerdo how can I help that this PR gets merged?

@tonerdo
Copy link
Owner

tonerdo commented Jul 10, 2018

Hi @manne, my sincere apologies for letting this slide for too long. I've been a bit swamped. I'm curious, in what instances will swapping out for a different constructor be beneficial? You'll run into issues because the return types are different from what is expected within the code. Will love to hear your thoughts

@manne
Copy link
Author

manne commented Apr 20, 2019

@tonerdo the design of the software for which I needed this, is/was sub optimal. Instead of utilizing dependency injection, an instance of a class, causing some serious side effects for (unit/integration) tests, is created deep down in the code. With this functionality the implementation can be changed, so no side effects will happen.

@jwdonahue
Copy link

@manne , wouldn't it be better to wrap the side-effects in methods and then just fake those?

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.

3 participants