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

feat: add inject_or #1376

Merged

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Aug 24, 2021

This PR removes the required parameter from Injector.inject and introduces inject_or that mimics the behavior of inject(..., required=False). The reasoning for this change is that type checkers take issue with using the result of inject(..., required=True) when the signature says that it could still be None. For example:

storage = context.inject(BaseStorage)
records = await storage.find_all_records(...)

Yields the error "find_all_records" is not a known member of "None" when using pyright, even though we know that the call will have resulted in an error if it was actually None.

This PR is mostly a proposal and does not yet fix the 109 or so instances of inject(..., required=False) that should be switched to inject_or. Open to suggestions or alternatives.

BREAKING CHANGE: `required` parameter has been removed from
Injector.inject

Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm dbluhm marked this pull request as draft August 24, 2021 21:16
@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2021

Codecov Report

Merging #1376 (28ae318) into main (102d9f1) will increase coverage by 0.00%.
The diff coverage is 99.28%.

@@           Coverage Diff           @@
##             main    #1376   +/-   ##
=======================================
  Coverage   95.28%   95.28%           
=======================================
  Files         476      476           
  Lines       29023    29048   +25     
=======================================
+ Hits        27654    27678   +24     
- Misses       1369     1370    +1     

@dbluhm
Copy link
Contributor Author

dbluhm commented Aug 31, 2021

Given no expressed opposition lol, I went ahead and finished the implementation on this to be able to consider these changes holistically.

@dbluhm dbluhm marked this pull request as ready for review August 31, 2021 21:23
Copy link
Contributor

@andrewwhitehead andrewwhitehead left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@andrewwhitehead andrewwhitehead merged commit 040b2f7 into openwallet-foundation:main Aug 31, 2021
@dbluhm dbluhm deleted the fix/injector-typing branch September 17, 2022 14:35
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

3 participants