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.

Sign up for GitHub

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

Jump to bottom

Inject named Autowired params when named args already present #14

Merged
merged 2 commits into from
May 12, 2020
Merged

Inject named Autowired params when named args already present #14

merged 2 commits into from
May 12, 2020

Conversation

craigminihan
Copy link
Contributor

@craigminihan craigminihan commented May 11, 2020 •
edited by roo-oliv
Loading

Fix #15.
Fixes the following:

@autowired
my_func(my_arg_1, my_arg_2, my_dep: AutoWired(...)): ...

my_func(my_arg_1=42, my_arg_2='hello world')

Previously injectable added my_dep as a positional causing a duplicate parameter error to be thrown.

craigminihan
craigminihan commented May 11, 2020
View reviewed changes
injectable/autowiring/autowired_decorator.py Outdated
@@ -65,11 +65,12 @@ def autowired(func: T) -> T:
def wrapper(*args, **kwargs):
bound_arguments = signature.bind_partial(*args, **kwargs).arguments
args = list(args)
caller_passed_kwargs = len(kwargs) > 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

detect any incoming named params

craigminihan
craigminihan commented May 11, 2020
View reviewed changes
injectable/autowiring/autowired_decorator.py Outdated
for parameter in autowired_parameters:
if parameter.name in bound_arguments:
continue
dependency = parameter.annotation.inject()
if parameter.kind is parameter.KEYWORD_ONLY:
if caller_passed_kwargs or (parameter.kind is parameter.KEYWORD_ONLY):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we already have named params we need to inject as named

craigminihan
craigminihan commented May 11, 2020
View reviewed changes
tests/unit/autowiring/autowired_decorator_unit_test.py Outdated
@@ -164,3 +164,24 @@ def f(a: AutowiredMockA, b: AutowiredMockB, c: AutowiredMockC):
assert parameters["a"] is None
assert parameters["b"] is AutowiredMockB.inject()
assert parameters["c"] is None

def test__autowired__injects_named_autowired_args_when_named_args_defined_by_the_caller(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is subtle variation on the previous test, reversing the b, c args showed the issue fixed by this change

@roo-oliv roo-oliv self-assigned this May 11, 2020
@roo-oliv roo-oliv added triage This issue or PR is marked for triage bug Something isn't working review This issue or PR is marked for review and removed triage This issue or PR is marked for triage labels May 11, 2020
@roo-oliv
Copy link
Owner

roo-oliv commented May 11, 2020 •
edited
Loading

Hi @craigminihan, thanks for the Pull Request with a reproducible code and unit tests. I believe a regression test is suitable for this too though I can add it myself :)

I was able to confirm the issue in injectable version 3.4.0 using Python 3.6.8.

I'm currently reviewing your PR. Your report concerns me in that there may be other issues with positional-only and keyword-only args being marked for autowiring. I'll dig into it and finish reviewing your Pull Request later today.

@roo-oliv roo-oliv mentioned this pull request May 12, 2020
Closed
@sonarcloud
Copy link

sonarcloud bot commented May 12, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

roo-oliv
roo-oliv approved these changes May 12, 2020
View reviewed changes
Copy link
Owner

@roo-oliv roo-oliv left a comment

Choose a reason for hiding this comment

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

@craigminihan this fix will be released in version 3.4.1 and should be available at PyPI today in a couple of minutes from now. Thanks for your help on this bug!

injectable/autowiring/autowired_decorator.py
Comment on lines -72 to +75
if parameter.kind is parameter.KEYWORD_ONLY:
kwargs[parameter.name] = dependency
else:
if parameter.kind is parameter.POSITIONAL_ONLY:
args.append(dependency)
else:
kwargs[parameter.name] = dependency
Copy link
Owner

Choose a reason for hiding this comment

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

@craigminihan I've inverted this logic to always inject parameters as named args except for when the parameter is positional only. This will fix the issue and play nicely with Python 3.8's positional-only args too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@allrod5 nice work! I'll check out 3.4.1 shortly

@roo-oliv roo-oliv merged commit 28fff24 into roo-oliv:master May 12, 2020
@roo-oliv roo-oliv added released The changes in this PR were released to PyPI and removed review This issue or PR is marked for review labels May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roo-oliv roo-oliv roo-oliv approved these changes

Assignees

@roo-oliv roo-oliv

Labels
bug Something isn't working released The changes in this PR were released to PyPI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Using named args breaks injectable
2 participants
@craigminihan @roo-oliv