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 a feature to override file extension defining compression #608

Closed
wants to merge 1 commit into from

Conversation

dmcguire81
Copy link
Contributor

Title

Add a feature to override file extension defining compression.

Motivation

- Implements #607

Tests

Added test_smart_open.S3OpenTest.test_rw_gzip_override and verified it fails as follows without the feature:

FAILED smart_open/tests/test_smart_open.py::S3OpenTest::test_rw_gzip_override  - OSError: Not a gzipped file...

Full test suite run output:

...
=============================================== short test summary info ===============================================
SKIPPED [1] smart_open/tests/test_package.py:18: requires missing dependencies
SKIPPED [1] smart_open/tests/test_package.py:13: requires missing dependencies
SKIPPED [1] smart_open/tests/test_package.py:24: requires missing dependencies
============================ 369 passed, 3 skipped, 7 warnings in 41.56s ==============================================

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

@mpenkov
Copy link
Collaborator

mpenkov commented Apr 20, 2021

Thank you for your interest in smart_open. I understand your concern about handling compression behavior.

I don't think adding yet another parameter is the way to go here, because there are already a lot of parameters to the open function. So, in my opinion, this PR is premature. Let's keep the ticket open and discuss alternative solutions there.

@mpenkov mpenkov closed this Apr 20, 2021
@dmcguire81
Copy link
Contributor Author

dmcguire81 commented Apr 20, 2021

As you mentioned, something that's not backwards compatible will require a major release. If that's not on the horizon, what is the objection to "yet another parameter", if it is backwards-compatible? More concretely, if you know that you have to break compatibility at the next major release, what is there to protect by not allowing a solution, in the meantime?

@mpenkov
Copy link
Collaborator

mpenkov commented Apr 21, 2021

It's more about cost-benefit than protection. For this PR, the cost is effort in reviewing, merging, documenting, explaining it to users, maintaining it and then eventually deprecating it once a proper solution is implemented. The benefit is immediately solving a problem that affects few people and does not strictly require immediate attention. It's simply not worth it.

I'd much rather invest that effort elsewhere, for example, into thinking of a proper way to solve the problem in #607.

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