[plaso] Replaced FromTimeParts in favor of dfdatetime and fixed #1700

[plaso] Replaced FromTimeParts in favor of dfdatetime and fixed #1700

[onager, 2018-01-19 04:51:07]

This CL doesn't seem to actually remove FromTimeParts from timelib, and leaves
the usage in the sccm parser alone - why?

https://codereview.appspot.com/332630043/diff/1/plaso/parsers/symantec.py
File plaso/parsers/symantec.py (right):

https://codereview.appspot.com/332630043/diff/1/plaso/parsers/symantec.py#new...
plaso/parsers/symantec.py:178: A Symantic log timestamp consist of six
hexadecimal octets, that represent:
Symantec

https://codereview.appspot.com/332630043/diff/1/plaso/parsers/syslog.py
File plaso/parsers/syslog.py (right):

https://codereview.appspot.com/332630043/diff/1/plaso/parsers/syslog.py#newco...
plaso/parsers/syslog.py:270: # TODO: add support for different precision.
What different precision?

https://codereview.appspot.com/332630043/diff/1/tests/parsers/syslog.py
File tests/parsers/syslog.py (right):

https://codereview.appspot.com/332630043/diff/1/tests/parsers/syslog.py#newco...
tests/parsers/syslog.py:59: self.assertEqual(event.timestamp,
expected_timestamp)
This makes the error messages substantially less useful, and more painful to
debug. Please refactor this.

[Joachim Metz, 2018-01-19 05:21:54]

> This CL doesn't seem to actually remove FromTimeParts from timelib, and leaves
> the usage in the sccm parser alone - why?

The reason is that the changes for sccm parser need additional work.

https://codereview.appspot.com/332630043/diff/1/plaso/parsers/symantec.py
File plaso/parsers/symantec.py (right):

https://codereview.appspot.com/332630043/diff/1/plaso/parsers/symantec.py#new...
plaso/parsers/symantec.py:178: A Symantic log timestamp consist of six
hexadecimal octets, that represent:
On 2018/01/19 04:51:07, onager wrote:
> Symantec

Done.

https://codereview.appspot.com/332630043/diff/1/plaso/parsers/syslog.py
File plaso/parsers/syslog.py (right):

https://codereview.appspot.com/332630043/diff/1/plaso/parsers/syslog.py#newco...
plaso/parsers/syslog.py:270: # TODO: add support for different precision.
left over due to less specific previous pyparsing definition; removed.

https://codereview.appspot.com/332630043/diff/1/tests/parsers/syslog.py
File tests/parsers/syslog.py (right):

https://codereview.appspot.com/332630043/diff/1/tests/parsers/syslog.py#newco...
tests/parsers/syslog.py:59: self.assertEqual(event.timestamp,
expected_timestamp)
> Please refactor this. 

Not in this CL. This is consistent with the rest of the codebase. If you want to
change this please create an issue and update the style guide.
https://github.com/log2timeline/plaso/wiki/Style-guide#tests

[Joachim Metz, 2018-01-19 05:31:15]

Code updated.

[onager, 2018-01-24 07:46:08]

Please also change the CL description, as the FromTimeParts function hasn't been
removed.

https://codereview.appspot.com/332630043/diff/20001/plaso/parsers/syslog.py
File plaso/parsers/syslog.py (right):

https://codereview.appspot.com/332630043/diff/20001/plaso/parsers/syslog.py#n...
plaso/parsers/syslog.py:152: pyparsing.Word(pyparsing.nums, exact=4) +
pyparsing.Literal('-') +
Why change this? This is much more difficult to understand, and to review.

In future, please try to limit independent refactoring in individual CLs.

https://codereview.appspot.com/332630043/diff/20001/plaso/parsers/syslog.py#n...
plaso/parsers/syslog.py:158: pyparsing.Word(pyparsing.nums, exact=6) +
pyparsing.oneOf(['-', '+']) +
Fractional seconds isn't specified to be to 6 decimal places in ISO8601. Please
fix.

[Joachim Metz, 2018-01-24 07:54:50]

https://codereview.appspot.com/332630043/diff/20001/plaso/parsers/syslog.py
File plaso/parsers/syslog.py (right):

https://codereview.appspot.com/332630043/diff/20001/plaso/parsers/syslog.py#n...
plaso/parsers/syslog.py:152: pyparsing.Word(pyparsing.nums, exact=4) +
pyparsing.Literal('-') +
Because it was incorrect before.

https://codereview.appspot.com/332630043/diff/20001/plaso/parsers/syslog.py#n...
plaso/parsers/syslog.py:158: pyparsing.Word(pyparsing.nums, exact=6) +
pyparsing.oneOf(['-', '+']) +
this is to match the log format not to match ISO8601

[Joachim Metz, 2018-01-24 07:56:11]

> Please also change the CL description, as the FromTimeParts function hasn't
been removed.

Deprecation != removal

https://en.wikipedia.org/wiki/Deprecation

[Joachim Metz, 2018-02-04 09:51:00]

Code updated.

[onager, 2018-02-11 12:12:28]

https://codereview.appspot.com/332630043/diff/20001/plaso/parsers/syslog.py
File plaso/parsers/syslog.py (right):

https://codereview.appspot.com/332630043/diff/20001/plaso/parsers/syslog.py#n...
plaso/parsers/syslog.py:152: pyparsing.Word(pyparsing.nums, exact=4) +
pyparsing.Literal('-') +
On 2018/01/24 07:54:50, Joachim Metz wrote:
> Because it was incorrect before.

Incorrect how?

https://codereview.appspot.com/332630043/diff/20001/plaso/parsers/syslog.py#n...
plaso/parsers/syslog.py:158: pyparsing.Word(pyparsing.nums, exact=6) +
pyparsing.oneOf(['-', '+']) +
On 2018/01/24 07:54:50, Joachim Metz wrote:
> this is to match the log format not to match ISO8601

OK, then please change the key name here, as it's confusing.

[Joachim Metz, 2018-02-11 13:26:14]

https://codereview.appspot.com/332630043/diff/20001/plaso/parsers/syslog.py
File plaso/parsers/syslog.py (right):

https://codereview.appspot.com/332630043/diff/20001/plaso/parsers/syslog.py#n...
plaso/parsers/syslog.py:152: pyparsing.Word(pyparsing.nums, exact=4) +
pyparsing.Literal('-') +
the previous definitions produced integers, the new approach needs a string.

https://codereview.appspot.com/332630043/diff/20001/plaso/parsers/syslog.py#n...
plaso/parsers/syslog.py:158: pyparsing.Word(pyparsing.nums, exact=6) +
pyparsing.oneOf(['-', '+']) +
On 2018/02/11 12:12:28, onager wrote:
> On 2018/01/24 07:54:50, Joachim Metz wrote:
> > this is to match the log format not to match ISO8601
> 
> OK, then please change the key name here, as it's confusing.

Done.

[onager, 2018-02-19 15:54:32]

LGTM, with small fixes

https://codereview.appspot.com/332630043/diff/40001/plaso/parsers/sccm.py
File plaso/parsers/sccm.py (right):

https://codereview.appspot.com/332630043/diff/40001/plaso/parsers/sccm.py#new...
plaso/parsers/sccm.py:77: 'hours': _ONE_OR_TWO_DIGITS.setResultsName('hours'),
Please revert this, per conversation

https://codereview.appspot.com/332630043/diff/40001/plaso/parsers/syslog.py
File plaso/parsers/syslog.py (right):

https://codereview.appspot.com/332630043/diff/40001/plaso/parsers/syslog.py#n...
plaso/parsers/syslog.py:164:
_PYPARSING_COMPONENTS['iso_8601_date'].setResultsName('iso_8601_date') +
Reminder- please change key name

[Joachim Metz, 2018-02-19 17:33:49]

https://codereview.appspot.com/332630043/diff/40001/plaso/parsers/sccm.py
File plaso/parsers/sccm.py (right):

https://codereview.appspot.com/332630043/diff/40001/plaso/parsers/sccm.py#new...
plaso/parsers/sccm.py:77: 'hours': _ONE_OR_TWO_DIGITS.setResultsName('hours'),
On 2018/02/19 15:54:31, onager wrote:
> Please revert this, per conversation

Done.

https://codereview.appspot.com/332630043/diff/40001/plaso/parsers/syslog.py
File plaso/parsers/syslog.py (right):

https://codereview.appspot.com/332630043/diff/40001/plaso/parsers/syslog.py#n...
plaso/parsers/syslog.py:164:
_PYPARSING_COMPONENTS['iso_8601_date'].setResultsName('iso_8601_date') +
On 2018/02/19 15:54:31, onager wrote:
> Reminder- please change key name

Done.

[Joachim Metz, 2018-02-19 17:39:11]

Code updated.

[Joachim Metz, 2018-02-19 17:45:47]

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