[dfdatetime] Added CopyToString functionality

[dfdatetime] Added CopyToString functionality

[onager, 2017-11-22 21:17:54]

https://codereview.appspot.com/335010043/diff/1/dfdatetime/cocoa_time.py
File dfdatetime/cocoa_time.py (right):

https://codereview.appspot.com/335010043/diff/1/dfdatetime/cocoa_time.py#newc...
dfdatetime/cocoa_time.py:95: YYYY-MM-DD hh:mm:ss.######[+-]##:##
The string seems not to have any timezone information, unlike what the docstring
says.

Please clarify what timezone the string is supposed to represent, and fix the
docstring.

https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py
File dfdatetime/interface.py (right):

https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
dfdatetime/interface.py:303: self, number_of_days, epoch_year, epoch_month,
epoch_day_of_month):
Rather than doing this, how about creating an Epoch object, then each class
having an EPOCH constant? It would clean up much of the error checking, and make
the calls to this method a lot clearer in parent classes.

https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
dfdatetime/interface.py:307: number_of_days (int): number of days.
Number of days since what? to what? Please expand this docstring, it's not clear
to me how to construct a correct call to this method as is.

https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
dfdatetime/interface.py:323: raise ValueError('Epock month value out of
bounds.')
Epoch

https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
dfdatetime/interface.py:467: """Retrieves the number of (remaining) days in a
century.
What do you mean by "remaining"? This doesn't seem to take into account how many
days have already passed in a given century.

https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
dfdatetime/interface.py:599: def CopyToString(self):
Please rename to __str__. This will enable much greater usability for
interactive environments (ipython, juypter) and matches python conventions.

Alternatively, if you plan multiple string representations,
CopyToISO8601String() or similar.

[Joachim Metz, 2017-11-23 06:23:29]

https://codereview.appspot.com/335010043/diff/1/dfdatetime/cocoa_time.py
File dfdatetime/cocoa_time.py (right):

https://codereview.appspot.com/335010043/diff/1/dfdatetime/cocoa_time.py#newc...
dfdatetime/cocoa_time.py:95: YYYY-MM-DD hh:mm:ss.######[+-]##:##
The intention is to add timezone support
https://github.com/log2timeline/dfdatetime/issues/72

I'll remove it from the docstring for now.

https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py
File dfdatetime/interface.py (right):

https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
dfdatetime/interface.py:303: self, number_of_days, epoch_year, epoch_month,
epoch_day_of_month):
I've made a note and think about it. I want to clean this up further also from a
performance perspective.

https://github.com/log2timeline/dfdatetime/issues/72

https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
dfdatetime/interface.py:307: number_of_days (int): number of days.
On 2017/11/22 21:17:54, onager wrote:
> Number of days since what? to what? Please expand this docstring, it's not
clear
> to me how to construct a correct call to this method as is.

Done.

https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
dfdatetime/interface.py:323: raise ValueError('Epock month value out of
bounds.')
On 2017/11/22 21:17:54, onager wrote:
> Epoch

Done.

https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
dfdatetime/interface.py:467: """Retrieves the number of (remaining) days in a
century.
left over from earlier version that did account for days have already passed in
a given century

https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
dfdatetime/interface.py:599: def CopyToString(self):
This not a method to create a string representation of the object but a "date
and time string". Seeing the object contains more information.

[onager, 2017-11-28 16:15:50]

On 2017/11/23 06:23:29, Joachim Metz wrote:
> https://codereview.appspot.com/335010043/diff/1/dfdatetime/cocoa_time.py
> File dfdatetime/cocoa_time.py (right):
> 
>
https://codereview.appspot.com/335010043/diff/1/dfdatetime/cocoa_time.py#newc...
> dfdatetime/cocoa_time.py:95: YYYY-MM-DD hh:mm:ss.######[+-]##:##
> The intention is to add timezone support
> https://github.com/log2timeline/dfdatetime/issues/72
> 
> I'll remove it from the docstring for now.
> 
> https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py
> File dfdatetime/interface.py (right):
> 
>
https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
> dfdatetime/interface.py:303: self, number_of_days, epoch_year, epoch_month,
> epoch_day_of_month):
> I've made a note and think about it. I want to clean this up further also from
a
> performance perspective.
> 
> https://github.com/log2timeline/dfdatetime/issues/72
> 
>
https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
> dfdatetime/interface.py:307: number_of_days (int): number of days.
> On 2017/11/22 21:17:54, onager wrote:
> > Number of days since what? to what? Please expand this docstring, it's not
> clear
> > to me how to construct a correct call to this method as is.
> 
> Done.
> 
>
https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
> dfdatetime/interface.py:323: raise ValueError('Epock month value out of
> bounds.')
> On 2017/11/22 21:17:54, onager wrote:
> > Epoch
> 
> Done.
> 
>
https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
> dfdatetime/interface.py:467: """Retrieves the number of (remaining) days in a
> century.
> left over from earlier version that did account for days have already passed
in
> a given century
> 
>
https://codereview.appspot.com/335010043/diff/1/dfdatetime/interface.py#newco...
> dfdatetime/interface.py:599: def CopyToString(self):
> This not a method to create a string representation of the object but a "date
> and time string". Seeing the object contains more information.

Is this missing a review update? I see you've addressed the comments, but no
changes...

[Joachim Metz, 2017-11-29 06:18:45]

Code updated.

[onager, 2017-11-29 11:33:40]

https://codereview.appspot.com/335010043/diff/20001/dfdatetime/fake_time.py
File dfdatetime/fake_time.py (right):

https://codereview.appspot.com/335010043/diff/20001/dfdatetime/fake_time.py#n...
dfdatetime/fake_time.py:80: YYYY-MM-DD hh:mm:ss
Please and an "or" here

https://codereview.appspot.com/335010043/diff/20001/dfdatetime/interface.py
File dfdatetime/interface.py (right):

https://codereview.appspot.com/335010043/diff/20001/dfdatetime/interface.py#n...
dfdatetime/interface.py:470: year (int): year e.g. 1970.
Add "year in the century", as things like 1901, 1970 etc. are all supposed to
return the same value.

https://codereview.appspot.com/335010043/diff/20001/dfdatetime/interface.py#n...
dfdatetime/interface.py:599: def CopyToString(self):
I still have question about this - what is the purpose of this method? If it's
not to have an " “informal” or nicely printable string representation" of the
object, what is it? 

If it's mean to be a standardized representation of datetime objects, what
standard is it? Is the standard defined by this docstring?

[Joachim Metz, 2017-11-29 20:03:27]

On 2017/11/29 11:33:40, onager wrote:
> https://codereview.appspot.com/335010043/diff/20001/dfdatetime/fake_time.py
> File dfdatetime/fake_time.py (right):
> 
>
https://codereview.appspot.com/335010043/diff/20001/dfdatetime/fake_time.py#n...
> dfdatetime/fake_time.py:80: YYYY-MM-DD hh:mm:ss
> Please and an "or" here
> 
> https://codereview.appspot.com/335010043/diff/20001/dfdatetime/interface.py
> File dfdatetime/interface.py (right):
> 
>
https://codereview.appspot.com/335010043/diff/20001/dfdatetime/interface.py#n...
> dfdatetime/interface.py:470: year (int): year e.g. 1970.
> Add "year in the century", as things like 1901, 1970 etc. are all supposed to
> return the same value.
> 
>
https://codereview.appspot.com/335010043/diff/20001/dfdatetime/interface.py#n...
> dfdatetime/interface.py:599: def CopyToString(self):
> I still have question about this - what is the purpose of this method? If it's
> not to have an " “informal” or nicely printable string representation" of the
> object, what is it? 
> 
> If it's mean to be a standardized representation of datetime objects, what
> standard is it? Is the standard defined by this docstring?

[Joachim Metz, 2017-11-29 20:03:33]

https://codereview.appspot.com/335010043/diff/20001/dfdatetime/fake_time.py
File dfdatetime/fake_time.py (right):

https://codereview.appspot.com/335010043/diff/20001/dfdatetime/fake_time.py#n...
dfdatetime/fake_time.py:80: YYYY-MM-DD hh:mm:ss
what about

... formatted as one of the following:

https://codereview.appspot.com/335010043/diff/20001/dfdatetime/interface.py
File dfdatetime/interface.py (right):

https://codereview.appspot.com/335010043/diff/20001/dfdatetime/interface.py#n...
dfdatetime/interface.py:470: year (int): year e.g. 1970.
On 2017/11/29 11:33:40, onager wrote:
> Add "year in the century", as things like 1901, 1970 etc. are all supposed to
> return the same value.

Done.

https://codereview.appspot.com/335010043/diff/20001/dfdatetime/interface.py#n...
dfdatetime/interface.py:599: def CopyToString(self):
So the object contains more information than just the date and time.

The string is human readable representation of the date and time only. It
implies some of the precision however not fully.

I rather not use Python object internals like __str__ and __unicode__ seeing
they don't port well between version 2 and 3 either.

[Joachim Metz, 2017-11-29 20:04:55]

Code updated.

[Joachim Metz, 2017-11-29 20:05:26]

https://codereview.appspot.com/335010043/diff/40001/tests/time_elements.py
File tests/time_elements.py (right):

https://codereview.appspot.com/335010043/diff/40001/tests/time_elements.py#ne...
tests/time_elements.py:1: #!/usr/bin/python
Note to self add testCopyToString to TimeElementsInMicroseconds

[Joachim Metz, 2017-12-10 11:18:41]

https://codereview.appspot.com/335010043/diff/40001/tests/time_elements.py
File tests/time_elements.py (right):

https://codereview.appspot.com/335010043/diff/40001/tests/time_elements.py#ne...
tests/time_elements.py:1: #!/usr/bin/python
On 2017/11/29 20:05:26, Joachim Metz wrote:
> Note to self add testCopyToString to TimeElementsInMicroseconds 

Done.

[Joachim Metz, 2017-12-10 11:19:40]

Code updated.

[Joachim Metz, 2017-12-10 11:22:39]

Code updated.

[onager, 2017-12-27 13:29:42]

https://codereview.appspot.com/335010043/diff/70029/dfdatetime/time_elements.py
File dfdatetime/time_elements.py (right):

https://codereview.appspot.com/335010043/diff/70029/dfdatetime/time_elements....
dfdatetime/time_elements.py:385: def CopyToString(self):
Rename this to CopyToDatetimeString, and I think this is OK to go.

[Joachim Metz, 2017-12-27 18:12:46]

https://codereview.appspot.com/335010043/diff/70029/dfdatetime/time_elements.py
File dfdatetime/time_elements.py (right):

https://codereview.appspot.com/335010043/diff/70029/dfdatetime/time_elements....
dfdatetime/time_elements.py:385: def CopyToString(self):
works for me but then we need to also change CopyFromString

[Joachim Metz, 2017-12-27 18:15:00]

https://codereview.appspot.com/335010043/diff/70029/dfdatetime/time_elements.py
File dfdatetime/time_elements.py (right):

https://codereview.appspot.com/335010043/diff/70029/dfdatetime/time_elements....
dfdatetime/time_elements.py:385: def CopyToString(self):
https://github.com/log2timeline/dfdatetime/issues/73

[onager, 2017-12-27 20:13:52]

On 2017/12/27 18:15:00, Joachim Metz wrote:
>
https://codereview.appspot.com/335010043/diff/70029/dfdatetime/time_elements.py
> File dfdatetime/time_elements.py (right):
> 
>
https://codereview.appspot.com/335010043/diff/70029/dfdatetime/time_elements....
> dfdatetime/time_elements.py:385: def CopyToString(self):
> https://github.com/log2timeline/dfdatetime/issues/73

OK, lets merge this then clean up both methods. LGTM

[Joachim Metz, 2017-12-27 20:28:20]

Code updated.

[Joachim Metz, 2017-12-27 20:30:24]

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