intg: Fix python3 issues #157
Conversation
|
Patch does make sense, but I don't feel comfortable ACKing python related stuff. |
src/tests/intg/files_ops.py
Outdated
| @@ -77,6 +77,7 @@ def _read_contents(self): | |||
|
|
|||
| def _write_contents(self, contents): | |||
| tmp_file = tempfile.NamedTemporaryFile(dir=self.tmp_dir, delete=False) | |||
There was a problem hiding this comment.
If content is unicode, then instead of encoding, NamedTemporaryFile can be opened in textual write mode "w", default mode is binary "w+b"
Would be nice to use with keyword with files, but unrelated to this patch
with tempfile.NamedTemporaryFile("w", dir=self.tmp_dir, delete=False) as tmp_file:
tmp_file.writelines(contents)
tmp_file.flush()There was a problem hiding this comment.
Good to know.
Fixed
|
@lslebodn: I'd say the changes mentioned by Martin (thanks for the review, btw) could (and do make sense) be part of this "series" (not exactly the same patch though, as already mentioned by Martin). |
|
There's no need to resend the patch, just change it before pushing ... |
|
IMO the |
|
I'd like to see the whole explanation in the commit message: Doesn't seem to be obvious as it ended up caught during the review (and I'm quite dumb on python related stuff). So better having it in the commit message than searching for it on my email if necessary. |
|
On (17/02/17 01:17), MartinBasti wrote:
IMO the `w` arg is quite obvious in python
Agree
The commit message should not learn people how to use python.
LS
|
|
It shouldn't teach people how to use python, true. But it has to have a good explanation why those changes where necessary (and here as much info as possible is desirable, mainly for people who don't do python and may blame this code for reference). |
NamedTemporaryFile use the default mode 'w+b'
and we tried to write strings. It is not a problem on python2
but failed on pyhton3
Python module ctypes directly uses C functions from libraries.
C functions usually expect/returns "char *" when string is expected.
But python3 uses unicode for string. Decoding returned bytes
("char *") to unicode strings simplify tests in python3.
Otherwise we would need to convert bytes to string in each assertion.
|
@lslebodn, thanks! Please, go for it. |
|
master:
* bac4458
LS
|
Mostly string/bytes related issues.
It looks like my comments to the integration tests
were lost as part of review or due to rebases.