Use built-in open whenever possible #208
Conversation
Changed the mock target to keep in step with the shortcut_open function. Split tests into separate test cases.
integration-tests/test_207.py
Outdated
| import sys | ||
|
|
||
| try: | ||
| import numpy as np |
There was a problem hiding this comment.
I think better to add numpy to test dependencies https://github.com/RaRe-Technologies/smart_open/blob/master/setup.py#L19
There was a problem hiding this comment.
Or even add it like standart unittest (not integration), wdyt?
There was a problem hiding this comment.
TBH, I don't think numpy belongs in our dependencies, because we only need it for one thing: to run this test.
What else would be the benefit of including it?
There was a problem hiding this comment.
because we only need it for one thing: to run this test.
yes, and I'm talking about test dependencies (not requirements of smart_open package)
What else would be the benefit of including it?
No, only for correct run test, that's all
integration-tests/test_207.py
Outdated
| dt = np.dtype([('time', [('min', int), ('sec', int)]), ('temp', float)]) | ||
| x = np.zeros((1,), dtype=dt) | ||
|
|
||
| fname = "test.dat" |
There was a problem hiding this comment.
better to use really tempfiles, instead of hardcoded names
| @@ -0,0 +1,28 @@ | |||
| import os | |||
There was a problem hiding this comment.
can you run this file in CI too (and other integration tests that missed) please?
There was a problem hiding this comment.
I added this test. There are no other tests that we can run automatically right now - the remaining tests require user input. They'll need a bit of work before they can be fully automatic.
There was a problem hiding this comment.
Aha, ok, we can do it later (out of scope of current PR)
smart_open/smart_open_lib.py
Outdated
| @@ -284,6 +284,16 @@ def _shortcut_open(uri, mode, **kw): | |||
| open_kwargs['encoding'] = encoding | |||
| mode = mode.replace('b', '') | |||
|
|
|||
| # | |||
| # Under Py3, the built-in open accepts kwargs, and it's OK to use that. | |||
| # Under Py2, the built-in open _doesn't_ accept kwargs, but we still use it | |||
There was a problem hiding this comment.
what's about buffering https://docs.python.org/2/library/functions.html#open ?
|
@mpenkov please use descriptive issue/PR titles & put |
- use tempfile instead of hard-coded file - integrate new test into travis.yml so that Travis runs it
|
@menshikh-iv I've responded to your comments. Please have a look. |
|
@mpenkov done |
|
LGTM. @menshikh-iv @mpenkov How about another release? This PR will be a welcome, important change. Btw, |
|
@piskvorky Thank you!! @menshikh-iv Is there anything left to do for this? If not, let's merge. |
|
@mpenkov LGTM for me too, thank you! |
Resolve issue #207