[plaso] Fix serializer for Python 3 and prevent Python 3 tests from hanging.

[plaso] Fix serializer for Python 3 and prevent Python 3 tests from hanging.

[Joachim Metz, 2018-02-26 15:04:18]

https://codereview.appspot.com/334530043/diff/1/plaso/storage/interface.py
File plaso/storage/interface.py (right):

https://codereview.appspot.com/334530043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:67: return None
why do you need an explict "return None" here ?

It is not needed, if it is explain why in a comment

https://codereview.appspot.com/334530043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:81: return None
None is returned by default and not needed

(repeat elsewhere)

https://codereview.appspot.com/334530043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:346: attribute_container =
self._serializer.ReadSerialized(
split this in 2 separate lines of code and catch unicode errors for
serialized_data.decode('utf-8')

(repeat elsewhere)

https://codereview.appspot.com/334530043/diff/1/tox.ini
File tox.ini (right):

https://codereview.appspot.com/334530043/diff/1/tox.ini#newcode13
tox.ini:13: {envbindir}/python ./run_tests.py
instead of adding {envbindir}/python change the shebang of ./run_tests.py
similar as in https://codereview.appspot.com/340320043/diff/1/run_tests.py

[ep, 2018-02-26 16:02:08]

On 2018/02/26 15:04:18, Joachim Metz wrote:
>
https://codereview.appspot.com/334530043/diff/1/plaso/storage/interface.py#ne...
> plaso/storage/interface.py:67: return None
> why do you need an explict "return None" here ?
> 
> It is not needed, if it is explain why in a comment

As I changed the file, the linter started complaining about issues that were
already there, and it seems to prefer having all explicit or all implicit
returns in a given function, but not a mixture of both
(https://github.com/PyCQA/pylint/issues/1267 quotes the part of PEP 8
recommending this.)

I'll address the other comments separately with a new patch set. Thanks!

[ep, 2018-02-27 08:23:57]

Code updated.

[ep, 2018-02-27 08:52:02]

Code updated.

[ep, 2018-02-27 08:52:56]

On 2018/02/26 15:04:18, Joachim Metz wrote:
>
https://codereview.appspot.com/334530043/diff/1/plaso/storage/interface.py#ne...
> plaso/storage/interface.py:346: attribute_container =
> self._serializer.ReadSerialized(
> split this in 2 separate lines of code and catch unicode errors for
> serialized_data.decode('utf-8')
> 
> (repeat elsewhere)

Done.

> https://codereview.appspot.com/334530043/diff/1/tox.ini
> File tox.ini (right):
> 
> https://codereview.appspot.com/334530043/diff/1/tox.ini#newcode13
> tox.ini:13: {envbindir}/python ./run_tests.py
> instead of adding {envbindir}/python change the shebang of ./run_tests.py
> similar as in https://codereview.appspot.com/340320043/diff/1/run_tests.py

Done.

[Joachim Metz, 2018-02-27 16:49:49]

> the linter started complaining about issues that were already there

weird, we use a chromium code style derivative and has not been an issue with
pylint before
which version of pylint are you using?

> but not a mixture of both (https://github.com/PyCQA/pylint/issues/1267 quotes
the part of PEP 8 recommending this.)

agree if we make it explicit, we should do it for the whole code base and update
the style guide as well

[ep, 2018-02-27 20:44:39]

On 2018/02/27 16:49:49, Joachim Metz wrote:
> weird, we use a chromium code style derivative and has not been an issue with
> pylint before
> which version of pylint are you using?

$ pylint --version
Using config file /usr/local/google/home/epuccia/gitdev/puccia/plaso/.pylintrc
pylint 1.8.2, 
astroid 1.6.1
Python 2.7.13 (default, Nov 24 2017, 17:33:09) 
[GCC 6.3.0 20170516]

> > but not a mixture of both (https://github.com/PyCQA/pylint/issues/1267
quotes
> the part of PEP 8 recommending this.)
> 
> agree if we make it explicit, we should do it for the whole code base and
update
> the style guide as well

I'm also happy to bypass the check (maybe by disabling
inconsistent-return-statements in .pylintrc) in the meantime. I thought I saw
this because the file hadn't been touched in a while and this check was
introduced in pylint only recently, so I went ahead by inertia with this :)

[Joachim Metz, 2018-02-27 20:51:40]

We're on 1.7.x, maybe try that instead of 1.8.x

https://github.com/log2timeline/plaso/wiki/Style-guide#linting

the pylint config and warnings keeps evolving

[ep, 2018-02-28 09:47:42]

Code updated.

[ep, 2018-02-28 09:48:24]

On 2018/02/27 20:51:40, Joachim Metz wrote:
> We're on 1.7.x, maybe try that instead of 1.8.x
> 
> https://github.com/log2timeline/plaso/wiki/Style-guide#linting
> 
> the pylint config and warnings keeps evolving

Sorry for the oversight. Done & updated.

[Joachim Metz, 2018-03-04 22:06:28]

one small nit, otherwise good to go

https://codereview.appspot.com/334530043/diff/60001/plaso/storage/interface.py
File plaso/storage/interface.py (right):

https://codereview.appspot.com/334530043/diff/60001/plaso/storage/interface.p...
plaso/storage/interface.py:345: serialized_str = serialized_data.decode('utf-8')
per https://github.com/log2timeline/plaso/wiki/Style-guide#naming please use
string instead of str

https://codereview.appspot.com/334530043/diff/60001/plaso/storage/interface.p...
plaso/storage/interface.py:546: serialized_str = serialized_data.decode('utf-8')
same comment as before

[ep, 2018-03-05 13:03:04]

Code updated.

[Joachim Metz, 2018-03-05 14:25:25]

LGTM, I'll check the tests and merge

[Joachim Metz, 2018-03-06 13:18:26]

tests are currently failing because of dfdatetime.

Let's wrap up https://codereview.appspot.com/337640043/, update dfdatetime and
then merge this

[Joachim Metz, 2018-03-07 01:34:39]

tests are already failing on the dfdatetime issue anyway. I'll break them and
then let's fix them ;)

[Joachim Metz, 2018-03-07 01:39:24]

Changes have been merged with master branch. To close the review and clean up
the feature branch you can run: review.py close python3