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

Propagate uri to compression_wrapper #842

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

ddelange
Copy link
Contributor

@ddelange ddelange commented Oct 16, 2024

Title

Please pick a concise, informative and complete title for your PR.
The title is important because it will appear in our change log.

Motivation

Please explain the motivation behind this PR in the description.

If you're fixing a bug, link to the issue number like so:

Fix #841

If you're adding a new feature, then consider opening a ticket and discussing it with the maintainers before you actually do the hard work.

Tests

If you're fixing a bug, consider test-driven development:

  1. Create a unit test that demonstrates the bug. The test should fail.
  2. Implement your bug fix.
  3. The test you created should now pass.

If you're implementing a new feature, include unit tests for it.

Make sure all existing unit tests pass.
You can run them locally using:

pytest smart_open

If there are any failures, please fix them before creating the PR (or mark it as WIP, see below).

Work in progress

If you're still working on your PR, include "WIP" in the title.
We'll skip reviewing it for the time being.
Once you're ready to review, remove the "WIP" from the title, and ping one of the maintainers (e.g. mpenkov).

Checklist

Before you create the PR, please make sure you have:

  • Picked a concise, informative and complete title
  • Clearly explained the motivation behind the PR
  • Linked to any existing issues that your PR will be solving
  • Included tests for any new functionality
  • Checked that all unit tests pass

Workflow

Please avoid rebasing and force-pushing to the branch of the PR once a review is in progress.
Rebasing can make your commits look a bit cleaner, but it also makes life more difficult from the reviewer, because they are no longer able to distinguish between code that has already been reviewed, and unreviewed code.

@ddelange
Copy link
Contributor Author

@mpenkov what do you think is happening here?

FAILED smart_open/tests/test_smart_open.py::SmartOpenHttpTest::test_http_bz2_query - AssertionError: assert b'BZh91AY&SYo...x90o|\x03\x1a' == b'Hello World...d Compressed.'

@ddelange
Copy link
Contributor Author

ddelange commented Oct 17, 2024

aha, splittext will fail to infer extension when the (http)URI has a query parameter appended to it, whereas the file_obj.name contains the relevant part of the URI only.

@ddelange ddelange mentioned this pull request Oct 17, 2024
5 tasks
@ddelange
Copy link
Contributor Author

ddelange commented Oct 17, 2024

I think #843 is a cleaner approach

scratch that, socket makefile gives a file object with an integer (fileno()) as name ref https://github.com/python/cpython/blob/v3.11.1/Lib/socket.py#L758

@mpenkov
Copy link
Collaborator

mpenkov commented Oct 26, 2024

Good work @ddelange ! Thank you.

@ddelange
Copy link
Contributor Author

anything else from my side here?

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.

FTP stream decompression - unable to transparently decompress
2 participants