Skip to content

Don't resolve passed parameters#63

Open
aikidojohn wants to merge 1 commit intokodemore:masterfrom
aikidojohn:fix_passed_parameter_resolution
Open

Don't resolve passed parameters#63
aikidojohn wants to merge 1 commit intokodemore:masterfrom
aikidojohn:fix_passed_parameter_resolution

Conversation

@aikidojohn
Copy link

When parameters are partially specified by the caller, parameter resolution happens for all parameters, including the passed in parameters. This can cause some unexpected behavior if the construction of the parameter is expensive or has side-effects.

I ran into this issue when introducing Kink into a complex project with a large unit test suit. The unit tests would pass in mock objects, but Kink would try to resolve them anyway, sometimes resulting in unexpected side-effects and errors.

This change will skip parameter resolution for parameters that were passed in by the caller.

** Maintainers - I am new to the Kink codebase. If there is a better way/place to make this change, I am happy to take feedback and see this change through.

@dkraczkowski
Copy link
Contributor

@aikidojohn Thanks for your contribution.

raise Exception("Constructing expensive object")

@inject
def greet(namer: ExpensiveObject, greeting: str = "Hello %s") -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

@aikidojohn, the fact that this works in such scenarios is a side effect I allowed in the initial design, but it was not my intention for the library to be used in this way. I find it concerning when functions are called without parameters, despite their signature indicating otherwise.

@dkraczkowski
Copy link
Contributor

@aikidojohn
I think the fix for not resolving parameters when they are passed is a good solution. However, I'd prefer using a scenario with constructor (initializer) injection instead of a function call. The reason is that I don’t want to encourage anyone to use the library in that way. If you know what you’re doing, that’s fine, but I’d rather not promote this approach.

@brownoxford
Copy link

@dkraczkowski Am I understanding your response correctly that it is expected behavior that the only way to override DI when calling a function is to pass ALL of the function arguments, even if some have defaults?

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