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

Full support for all async iterables/iterators as inputs and sources (even if they don't have an aclose()) #19

Merged
merged 4 commits into from
Apr 16, 2024

Conversation

mikenerone
Copy link
Contributor

@mikenerone mikenerone commented Feb 8, 2024

In _utils.py, this branch provides safe_closing() (only meant for Slurry's use, so no doc changes). It can be used in place of the async_generator.aclosing() context manager that was used frequently previously, with two differences that make it safer (and also type-checker friendly as it is correctly typed as supporting all these cases):

  1. It now works with any async iterable. Previously, it would error on one that didn't act as its own async iterator object. Granted, this is unusual, but taking the care makes type checkers happy.
  2. It now works with any async iterator. Previously, it would error on one that didn't have an aclose() method, which is only somewhat unusual.

Also provided is a safe_aclose() async function intended to be used anywhere an explicit aclose() method call is desired, but with the benefit of item 2 above, so that it's safe to use without checking for the presence of aclose().

All of the input async gens that previously lived in tests/fixtures.py are now set up as actual pytest fixtures (in tests/conftest.py). This allows them to yield both "with aclose()" and "without aclose()" versions of themselves so that all existing tests that use them (with a small signature update) automatically generate tests for both cases (test count went from 43 to 76 to cover all these cases).

As a result of using safe_closing() everywhere, the async_generator dependency is no longer needed, and has been removed.

Note: also applying to outputs so async_generator dep no longer needed.
@andersea
Copy link
Owner

Thanks for this. Haven't had time to do anything but a superficial look over the first couple of changes. The changes look ok so far. Hopefully I can get a moment to go through everything this weekend.

@andersea andersea self-assigned this Feb 15, 2024
Copy link
Owner

@andersea andersea left a comment

Choose a reason for hiding this comment

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

Going through this was quite a bit more challenging than I anticipated. I have never used test fixtures in pytest, so it was a lot to wrap my head around. Took a bit headscratching and some research to figure out. I wonder if maybe the same could be achieved with a simpler approach than creating a custom fixture function wrapper?

The other main comment is, that there seems to be some test code that is now redundant, because of the new fixtures, which could be removed. See the comments on the individual files.

tests/test_filters.py Show resolved Hide resolved
tests/test_pipeline.py Show resolved Hide resolved
tests/test_threading.py Show resolved Hide resolved
tests/fixtures.py Show resolved Hide resolved
@mikenerone
Copy link
Contributor Author

Going through this was quite a bit more challenging than I anticipated. I have never used test fixtures in pytest, so it was a lot to wrap my head around. Took a bit headscratching and some research to figure out. I wonder if maybe the same could be achieved with a simpler approach than creating a custom fixture function wrapper?

I appreciate you putting in the time! To answer your question, the other approach would be to duplicate all of the tests to cover both cases separately. While that's "simpler" in some way, it's a maintainability trade-off because it's much less DRY, the risk being that when making a test update, one misses one of the two places. IMO maintainability and consistency should be the overriding concerns, but I'll of course defer to your wishes if you prefer the explicitly duplicated tests.

@andersea andersea merged commit c4fef46 into andersea:master Apr 16, 2024
6 checks passed
@andersea
Copy link
Owner

Merged! Thanks!

And sorry for the extremely long delay! I went to Texas for the eclipse for two weeks and just came back this weekend and before that, prep for travel and work took up all my time.

@mikenerone
Copy link
Contributor Author

Merged! Thanks!

And sorry for the extremely long delay! I went to Texas for the eclipse for two weeks and just came back this weekend and before that, prep for travel and work took up all my time.

Hehe, no worries. I was also in Texas (where I live) for the eclipse. Hadn't seen a cloud for three months prior. Suddenly overcast that day.

@mikenerone mikenerone deleted the mikenerone/safe_aclosing branch April 19, 2024 17:29
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.

2 participants