Update cStringIO imports by rahulporuri · Pull Request #95 · enthought/apptools

rahulporuri · 2019-07-03T08:27:25Z

fixes #94

modified: apptools/logger/agent/attachments.py modified: apptools/logger/log_point.py modified: apptools/logger/plugin/logger_service.py

codecov-io · 2019-07-03T08:36:19Z

Codecov Report

❗ No coverage uploaded for pull request base (master@30cd14d). Click here to learn what that means.
The diff coverage is 87.5%.

@@            Coverage Diff            @@
##             master      #95   +/-   ##
=========================================
  Coverage          ?   41.68%           
=========================================
  Files             ?      240           
  Lines             ?     8912           
  Branches          ?     1158           
=========================================
  Hits              ?     3715           
  Misses            ?     5045           
  Partials          ?      152

Impacted Files	Coverage Δ
apptools/logger/log_point.py	`0% <0%> (ø)`
apptools/logger/agent/attachments.py	`81.35% <100%> (ø)`
apptools/logger/plugin/logger_service.py	`65.88% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30cd14d...327ef6a. Read the comment docs.

mdickinson · 2019-07-03T15:58:18Z


        import zipfile
-        from cStringIO import StringIO
+        from six.moves import cStringIO as StringIO


Would the simpler from six import StringIO work here?

Actually, looking at the place where this is used, do we need BytesIO rather than StringIO?

Looks like we do. On Python 3, working in a directory that contains a tox.ini file, I see this:

>>> z = zipfile.ZipFile(StringIO(), "w") >>> z.write("tox.ini") Exception ignored in: <function ZipFile.__del__ at 0x1076fe7b8> Traceback (most recent call last): File "/opt/local/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/zipfile.py", line 1789, in __del__ self.close() File "/opt/local/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/zipfile.py", line 1807, in close self._write_end_record() File "/opt/local/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/zipfile.py", line 1911, in _write_end_record self.fp.write(endrec) TypeError: string argument expected, got 'bytes' Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/opt/local/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/zipfile.py", line 1743, in write with open(filename, "rb") as src, self.open(zinfo, 'w') as dest: File "/opt/local/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/zipfile.py", line 1470, in open return self._open_to_write(zinfo, force_zip64=force_zip64) File "/opt/local/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/zipfile.py", line 1583, in _open_to_write self.fp.write(zinfo.FileHeader(zip64)) TypeError: string argument expected, got 'bytes'

mdickinson

I think we need to use io.BytesIO (on both Python 2 and Python 3) for the two cases where we're writing a zip file. We could use six.BytesIO, but io.BytesIO is a bit more correct here: six.BytesIO will accept Unicode text on Python 2, but io.BytesIO doesn't.

For the other case, I'd suggest six.StringIO as a shorter way to spell six.moves.cStringIO. In that case, I think we do want six.StringIO rather than io.StringIO, for the leniency of allowing either str or unicode to be written on Python 2.

mdickinson · 2019-07-03T16:12:22Z

And this also shows that unfortunately this code isn't being exercised by unit tests. Does it look possible to write a regression test? (And one that would catch the accidental misuse of StringIO instead of BytesIO for the zip files?)

when used with zipfile.ZipFile, we need a BytesIO and not a StringIO object. also update the imports to be less verbose modified: apptools/logger/agent/attachments.py modified: apptools/logger/log_point.py modified: apptools/logger/plugin/logger_service.py

modified: apptools/logger/agent/attachments.py new file: apptools/logger/agent/tests/__init__.py new file: apptools/logger/agent/tests/test_attachments.py new file: apptools/logger/plugin/tests/__init__.py new file: apptools/logger/plugin/tests/test_logger_service.py

rahulporuri · 2019-10-08T17:04:06Z

@mdickinson i was going to say that writing tests will take too long but it didn't really. can you take another look at the PR? I think i've addressed all of the comments and i've added the regression tests (to atleast catch trivial bugs).

rahulporuri · 2019-10-08T17:07:26Z

======================================================================

ERROR: test_attaching_single_project (apptools.logger.agent.tests.test_attachments.AttachmentsTestCase)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/home/travis/build/enthought/apptools/apptools/logger/agent/tests/test_attachments.py", line 25, in setUp

    filehandle.write("Dummy data in dummy file for dummies")

TypeError: write() argument 1 must be unicode, not str

======================================================================

ERROR: test_attaching_workspace (apptools.logger.agent.tests.test_attachments.AttachmentsTestCase)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/home/travis/build/enthought/apptools/apptools/logger/agent/tests/test_attachments.py", line 25, in setUp

    filehandle.write("Dummy data in dummy file for dummies")

TypeError: write() argument 1 must be unicode, not str

modified: apptools/logger/agent/tests/test_attachments.py

modified: apptools/logger/agent/attachments.py modified: apptools/logger/agent/tests/test_attachments.py modified: apptools/logger/plugin/logger_service.py

modified: apptools/logger/agent/tests/test_attachments.py

mdickinson · 2019-10-09T13:41:44Z

+            msg = logger_service.create_email_message(
+                fromaddr='', toaddrs='', ccaddrs='', subject='', priority=''
+            )
+        self.assertIsNotNone(msg)


I'd drop this, unless there's some a priori reason to expect the msg to be None. This check is redundant with the assertIsInstance check below.

mdickinson · 2019-10-09T13:41:57Z

+                fromaddr='', toaddrs='', ccaddrs='', subject='', priority='',
+                include_userdata=True,
+            )
+        self.assertIsNotNone(msg)


Same comment as above.

mdickinson · 2019-10-09T13:42:05Z

+                include_userdata=True,
+            )
+        self.assertIsNotNone(msg)
+        self.assertIsInstance(msg, MIMEMultipart)


Missing newline!

mdickinson

Done with review; various nitpicks, but looks good otherwise.

modified: apptools/logger/plugin/tests/test_logger_service.py

FIX : Update cStringIO imports

4ff8d88

modified: apptools/logger/agent/attachments.py modified: apptools/logger/log_point.py modified: apptools/logger/plugin/logger_service.py

mdickinson reviewed Jul 3, 2019

View reviewed changes

mdickinson requested changes Jul 3, 2019

View reviewed changes

poruri sai rahul added 2 commits October 8, 2019 17:07

Fix the test, pass unicode instead of str

0d75bd9

modified: apptools/logger/agent/tests/test_attachments.py