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

Update __init__.py #127

Closed
wants to merge 1 commit into from
Closed

Update __init__.py #127

wants to merge 1 commit into from

Conversation

HolimaX
Copy link

@HolimaX HolimaX commented Oct 13, 2024

@jaraco
Copy link
Owner

jaraco commented Nov 10, 2024

Thanks for the suggestion. Unfortunately, the PR doesn't describe the issue, only declares that it's a resource leak. Better would be to file an issue describing how there's a resource leak and under what conditions and why that's a problem.

Even the commit message is meaningless. The commit message should at least describe what change is happening and some rationale for the change (or link to an issue with the rationale).

More importantly, however, is that the proposed fix doesn't even work. By closing the stream in a with context, the return value of the method is no longer valid, which is evidenced by many tests failing. I think it may not be possible to close this resource reliably.

Thanks again for the attempt, and please feel encouraged to provide more context and propose a different workable solution.

@jaraco jaraco closed this Nov 10, 2024
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