Unified method for text diff for all Content types

For text-like content types return a text diff and for all other types return an empty list.

Useful for a homogeneous treatment of item contents while creating a content diff for a mail notification.

[Thomas.J.Waldmann, 2013-09-05 21:08:53]

https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/_tests/test_Con...
File MoinMoin/items/_tests/test_Content.py (right):

https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/_tests/test_Con...
MoinMoin/items/_tests/test_Content.py:281: assert difflines == []
? how should this work at all for non-text?

https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/content.py
File MoinMoin/items/content.py (right):

https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/content.py#newc...
MoinMoin/items/content.py:57: from MoinMoin.util.diff_text import diff
look here.

https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/content.py#newc...
MoinMoin/items/content.py:278: :param newdata: new content string
string sounds a bit like text. either leave it away, or say "content data
(bytes)" if you mean that.

https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/content.py#newc...
MoinMoin/items/content.py:279: :return: list of diffs
too unprecise.

https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/content.py#newc...
MoinMoin/items/content.py:853: from MoinMoin.util.diff_html import diff
and here ...

https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/content.py#newc...
MoinMoin/items/content.py:865: def _get_diff_text(self, olddata, newdata):
and here ...

why not be more consistent with naming the method like the other, with param
names, with where import is done, ...

[ana.balica, 2013-09-05 21:27:12]

https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/_tests/test_Con...
File MoinMoin/items/_tests/test_Content.py (right):

https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/_tests/test_Con...
MoinMoin/items/_tests/test_Content.py:281: assert difflines == []
On 2013/09/05 21:08:53, Thomas.J.Waldmann wrote:
> ? how should this work at all for non-text?

For non text types we get an empty list. In that case the section with Content
data from notification mail is not rendered at all. All we give is the link to
the diff on wiki.

https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/content.py
File MoinMoin/items/content.py (right):

https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/content.py#newc...
MoinMoin/items/content.py:279: :return: list of diffs
On 2013/09/05 21:08:53, Thomas.J.Waldmann wrote:
> too unprecise.

'list of diffs in a unified format'

is that somehow better?

https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/content.py#newc...
MoinMoin/items/content.py:865: def _get_diff_text(self, olddata, newdata):
On 2013/09/05 21:08:53, Thomas.J.Waldmann wrote:
> and here ...
> 
> why not be more consistent with naming the method like the other, with param
> names, with where import is done, ...

Is `_get_data_diff_text` better?
For params I made the analogy with params like oldrev and newrev. What would be
a better replacement for olddata and newdata?

[Thomas.J.Waldmann, 2013-09-05 21:38:04]

On 2013/09/05 21:27:12, ana.balica wrote:
>
https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/_tests/test_Con...
> File MoinMoin/items/_tests/test_Content.py (right):
> 
>
https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/_tests/test_Con...
> MoinMoin/items/_tests/test_Content.py:281: assert difflines == []
> On 2013/09/05 21:08:53, Thomas.J.Waldmann wrote:
> > ? how should this work at all for non-text?
> 
> For non text types we get an empty list. In that case the section with Content
> data from notification mail is not rendered at all. All we give is the link to
> the diff on wiki.
> 
> https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/content.py
> File MoinMoin/items/content.py (right):
> 
>
https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/content.py#newc...
> MoinMoin/items/content.py:279: :return: list of diffs
> On 2013/09/05 21:08:53, Thomas.J.Waldmann wrote:
> > too unprecise.
> 
> 'list of diffs in a unified format'
> 
> is that somehow better?

you should mention it is a list of LINES and also whether they have trailing
linefeeds or not.
so callers know what they get.

> 
>
https://codereview.appspot.com/13418045/diff/1/MoinMoin/items/content.py#newc...
> MoinMoin/items/content.py:865: def _get_diff_text(self, olddata, newdata):
> On 2013/09/05 21:08:53, Thomas.J.Waldmann wrote:
> > and here ...
> > 
> > why not be more consistent with naming the method like the other, with param
> > names, with where import is done, ...
> 
> Is `_get_data_diff_text` better?

yes.

> For params I made the analogy with params like oldrev and newrev. What would
be
> a better replacement for olddata and newdata?

see the other method.

[Thomas.J.Waldmann, 2013-09-06 15:10:51]

https://codereview.appspot.com/13418045/diff/15001/MoinMoin/items/_tests/test...
File MoinMoin/items/_tests/test_Content.py (right):

https://codereview.appspot.com/13418045/diff/15001/MoinMoin/items/_tests/test...
MoinMoin/items/_tests/test_Content.py:11: from StringIO import StringIO
maybe rather use the py3-compatible classes from io lib for new code, e.g.
BytesIO.

https://codereview.appspot.com/13418045/diff/15001/MoinMoin/items/_tests/test...
MoinMoin/items/_tests/test_Content.py:284: newfile.seek(0)
if that is for getting the file to the initial state, maybe rather create the
file in the loop. as it is in memory, it's cheap and needs less thinking than
those seeks...

https://codereview.appspot.com/13418045/diff/15001/MoinMoin/items/content.py
File MoinMoin/items/content.py (right):

https://codereview.appspot.com/13418045/diff/15001/MoinMoin/items/content.py#...
MoinMoin/items/content.py:274: def _get_data_diff_text(self, oldfile, newfile):
there is only THIS method here now in that class.

https://codereview.appspot.com/13418045/diff/15001/MoinMoin/items/content.py#...
MoinMoin/items/content.py:856: storage_item = flaskg.storage[self.name]
interesting, just noticed that storage_item isn't used?

https://codereview.appspot.com/13418045/diff/15001/MoinMoin/items/content.py#...
MoinMoin/items/content.py:861: newrev=newrev,
but the template might access the revisions directly.

you could also use a (text) template for the _text method, btw. to make the
methods more similar...

https://codereview.appspot.com/13418045/diff/15001/MoinMoin/items/content.py#...
MoinMoin/items/content.py:874: return diff(old_text.splitlines(),
new_text.splitlines())
looks ok, except that we now have 2 different apis depending on whether we want
text or html.

do you plan to change the other one also in a separate cs, so they are in sync
again? _get_data_diff_html

[ana.balica, 2013-09-06 17:27:20]

https://codereview.appspot.com/13418045/diff/15001/MoinMoin/items/content.py
File MoinMoin/items/content.py (right):

https://codereview.appspot.com/13418045/diff/15001/MoinMoin/items/content.py#...
MoinMoin/items/content.py:874: return diff(old_text.splitlines(),
new_text.splitlines())
On 2013/09/06 15:10:51, Thomas.J.Waldmann wrote:
> do you plan to change the other one also in a separate cs, so they are in sync
> again? _get_data_diff_html

Ok, there is only _get_data_diff_html method to be changed, so it's not much.
Though the method is called in methods like `_render_data_diff(self, oldrev,
newrev)`. I assume their interface will stay the same.

[Thomas.J.Waldmann, 2013-09-07 18:19:25]

https://codereview.appspot.com/13418045/diff/15001/MoinMoin/items/content.py
File MoinMoin/items/content.py (right):

https://codereview.appspot.com/13418045/diff/15001/MoinMoin/items/content.py#...
MoinMoin/items/content.py:874: return diff(old_text.splitlines(),
new_text.splitlines())
well, you said that you can't do what you want with that interface, so it'll
either need change or would stay inconsistent.