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

Add os.walk wrapper #193

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

stankudrow
Copy link

@stankudrow stankudrow commented Nov 11, 2024

Addresses the issue #160 .

Features:

  • Now the wrap function is placed in the base.py module as a basic function of sorts -> importing the wrap function from the os.py or ospath.py are not expected to be down
  • the wrapped functions got arranged alphabetically and form sections split by new lines
  • added a test on the wrapped os.walk coroutine

@stankudrow
Copy link
Author

stankudrow commented Nov 12, 2024

Blocked by the PR #192 .

Update: not any more.

@stankudrow stankudrow marked this pull request as draft November 12, 2024 08:28
@stankudrow stankudrow marked this pull request as ready for review November 12, 2024 09:14
@stankudrow stankudrow changed the title Add os.listdir and os.walk wrappers Add os.walk wrapper Nov 12, 2024
@stankudrow
Copy link
Author

#160 (comment) - the os.listdir already wrapped, the os.walk added.

@stankudrow
Copy link
Author

stankudrow commented Nov 12, 2024

@Tinche , hello. Rebased and ready for review, all checks have been passed.

Update: I found the issue #167 on an "invented-here" solution, what are your suggestions?

@stankudrow
Copy link
Author

@Tinche , hello, shall we review this PR, please?

@Tinche
Copy link
Owner

Tinche commented Dec 4, 2024

Apologies. I have a small baby nowadays.

So the original sync os.walk is a generator, right? This has some very useful properties which we should keep. Can we make the async version an async generator? Each step can do a step of the sync one.

@stankudrow
Copy link
Author

Apologies. I have a small baby nowadays.

So the original sync os.walk is a generator, right? This has some very useful properties which we should keep. Can we make the async version an async generator? Each step can do a step of the sync one.

Hello and no need for apologies, au contraire, congratulations.

@stankudrow
Copy link
Author

@iiSeymour , @alemigo , hello. Could you possibly help with reviewing?

@stankudrow
Copy link
Author

@Tinche , hello. If you have time, could you assign a reviewer for this PR? Thank you and I wish you splendid holidays.

src/aiofiles/ospath.py Outdated Show resolved Hide resolved
statvfs = wrap(os.statvfs)


async def walk(top, topdown=True, onerror=None, followlinks=False):
Copy link
Owner

Choose a reason for hiding this comment

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

Ok, better, but this function isn't really async now. It doesn't handle file system operations in a thread, and so can block the event loop, which we're trying to avoid.

Instead of a simple for loop, try executing next() on the threadpool.

To get you started, maybe try something like

source = os.walk(...)

while True:
    f = await loop.run_in_executor(None, next, source)

@stankudrow stankudrow requested a review from Tinche December 28, 2024 19:15
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