adriangb / di

Pythonic dependency injection
https://www.adriangb.com/di/
MIT License
301 stars 13 forks source link

Fix/callable class generator not handled #108

Closed maxzhenzhera closed 1 year ago

maxzhenzhera commented 1 year ago

Closes #106

maxzhenzhera commented 1 year ago

Check Version Bump

I did a few PRs and I don't know how to bump new versions concurrently

maxzhenzhera commented 1 year ago

Tests

Again Error uploading to [https://codecov.io:](https://codecov.io/) As in #103

maxzhenzhera commented 1 year ago

Also, please, look at the tests/test_execute.py, because I found here some strange things: https://github.com/adriangb/di/blob/f8b0f4b38e6f43c4b5365bac1c663b64a60afefd/tests/test_execute.py#L121-L124 In many places anyio.Event.set() is not awaited:

This test: https://github.com/adriangb/di/blob/f8b0f4b38e6f43c4b5365bac1c663b64a60afefd/tests/test_execute.py#L167-L206 Locally, I tried to fix question about awaiting asyncio.Event.set() - but seems it does not help.

I haven't paid a lot of attention to how this works and how it should be - but I randomly changed sync1 and sync2 in multiple mark places => and tests haven't failed.

So, it looks like the test does not work as expected and it is already broken.

Note: Maybe it is somehow related to a big copy-past that I deleted in PR (https://github.com/adriangb/di/pull/108/commits/62a3d8b60f3a5fcdc12313767dd201ea9cc58678)

codecov-commenter commented 1 year ago

Codecov Report

Merging #108 (d4d09d6) into main (4d94386) will increase coverage by 0.40%. The diff coverage is 100.00%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
+ Coverage   96.57%   96.98%   +0.40%     
==========================================
  Files          67       67              
  Lines        2280     2286       +6     
  Branches      331      333       +2     
==========================================
+ Hits         2202     2217      +15     
+ Misses         68       59       -9     
  Partials       10       10              
Files Changed Coverage Δ
di/_utils/inspect.py 100.00% <100.00%> (ø)
tests/test_execute.py 98.23% <100.00%> (+3.37%) :arrow_up:
tests/test_task.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

maxzhenzhera commented 1 year ago

Found breaking commit: https://github.com/adriangb/di/commit/bf35a5acea3231d8278b4f4f3e97d2d44b50e1c5 (Removed handling of callable classes).


In many places anyio.Event.set() is not awaited:

I'm sorry for being wrong. Actually, my PyCharm highlighted it as awaitable (but looking in the source I understood that it is deprecated).

I haven't paid a lot of attention to how this works and how it should be - but I randomly changed sync1 and sync2 in multiple mark places => and tests haven't failed.

As I found out - sync params are just obsolete params: https://github.com/adriangb/di/commit/e43def722d1f3d6a5fce29ae5e367930c615288c So, I fully removed these parameters from the test parametrize.


Some additional notes about test_concurrency_async:


So, I propose (idea for another PR):


Finally, test_concurrency.py (https://github.com/adriangb/di/blob/main/tests/test_concurrency.py) Since as_async is used only in tests - test_concurrency.py looks like testing test code. Sounds kind of strange.

maxzhenzhera commented 1 year ago

@adriangb, please review