add support for local file ignore_extension #173
Conversation
There was a problem hiding this comment.
Good work @shmuelamar!
Looks good for me, need to add several changes
- Add test for case when
ignore_extension=True - (Maybe?) Revert current changes for tests (with explicit
ignore_extension=False) - Add information about this feature to
README.mdwith example
CC: @mpenkov can you review too please
smart_open/smart_open_lib.py
Outdated
| _, ext = os.path.splitext(filename) | ||
| if ext == '.bz2': | ||
| return ClosingBZ2File(file_obj, mode) | ||
| elif ext == '.gz': | ||
| if ext == '.gz': |
| @@ -292,29 +292,29 @@ def test_file(self, mock_smart_open): | |||
| smart_open_object = smart_open.smart_open(prefix+full_path, read_mode) | |||
| smart_open_object.__iter__() | |||
| # called with the correct path? | |||
| mock_smart_open.assert_called_with(full_path, read_mode, encoding=None, errors='strict') | |||
| mock_smart_open.assert_called_with(full_path, read_mode, encoding=None, errors='strict', ignore_extension=False) | |||
There was a problem hiding this comment.
Need also add a test for check this feature (where ignore_extension=True)
|
ping @shmuelamar, are you plan to finish this PR? |
|
yes @menshikh-iv ill try finish it this week - got busy week. |
shmuelamar
force-pushed
the
master
branch
2 times, most recently
from
080fcee to
a46e0e8
Compare
|
@menshikh-iv just finished completing the tests and reverting the if/else change. regarding the revert current changes to test (explicit ignore_extension), i think its not quite possible without changing the implementation itself as I'm open to suggestions, please LMK if I should anything else. thanks :) shmulik |
There was a problem hiding this comment.
Looks good for me (only several questions), @mpenkov please have a look too
| @@ -215,7 +219,7 @@ def smart_open(uri, mode="rb", **kw): | |||
| raise TypeError('don\'t know how to handle uri %s' % repr(uri)) | |||
|
|
|||
|
|
|||
| def s3_open_uri(parsed_uri, mode, **kwargs): | |||
| def s3_open_uri(parsed_uri, mode, ignore_extension=False, **kwargs): | |||
There was a problem hiding this comment.
Should ignore_extension be an explicit parameter (or this should be part of kwargs), same question for s3_open_key, @mpenkov thought?
| mock_smart_open.assert_called_with(full_path, read_mode, encoding=None, errors='strict') | ||
| mock_smart_open.assert_called_with(full_path, read_mode, encoding=None, errors='strict', ignore_extension=False) | ||
|
|
||
| @mock.patch('smart_open.smart_open_lib.file_smart_open') |
There was a problem hiding this comment.
should it be maybe_.. decorator instead?
|
hey @menshikh-iv @mpenkov pls LMK how can I help merge this PR. is there anything else to fix or answer? |
|
@shmuelamar we'll merge #185 first and after - current PR, don't worry. |
|
@shmuelamar can you make rebase with current master, please? |
|
The current master handles ignore_extension across all interfaces. It's now in one place: https://github.com/RaRe-Technologies/smart_open/blob/master/smart_open/smart_open_lib.py#L195 @shmuelamar please have a look there, it should help with your rebase. |
|
ping @shmuelamar, what's about PR? |
|
Ping @shmuelamar, are you planning to finish PR? |
|
Abandoned. Furthermore, the current code seems to include the feature mentioned in this PR. |
fix for #172 , few notes:
ClosingGzipFileclass for S3 instead of plainGzipFile. reviewing the python3 code forGzipFileit seems to not affect but its something worth reviewing to see if it affects something (it even might be better). LMK if you want me to use for S3 the plain stdlibGzipFileand ill change the implementation.ignore_extensionto docs, to find it in the first place I dig into the source.