Issue 32: IM messages don't send permalinks...

Issue 32: IM messages don't send permalinks

Extensive rewrite of the IM rendering code. The output to the
backend is now both in plain text and html.

Patch by: Jonas Nockert

Committed in r51

[adewale]

http://rietku.appspot.com/3001/diff/1/2
File common/templatetags/format.py (right):

http://rietku.appspot.com/3001/diff/1/2#newcode109
Line 109: try:
Can you extract out:
+ try:
+ max_len = int(max_len)
+ except (TypeError, ValueError):
+ max_len = 30

as a method that is reused in both truncate() and linked_entry_truncated_title()

http://rietku.appspot.com/3001/diff/1/2#newcode114
Line 114: if len(value) >= max_len:
Can you add a test for this method. Especially the branch of the code that deals
with u"\u2026" as it's not obvious what that's trying to do.

[lemonad]

Thanks for the comments, I'll get right on it.

http://rietku.appspot.com/3001/diff/1/2
File common/templatetags/format.py (right):

http://rietku.appspot.com/3001/diff/1/2#newcode109
Line 109: try:
On 2009/03/27 20:37:58, adewale wrote:
> Can you extract out:
> + try:
> + max_len = int(max_len)
> + except (TypeError, ValueError):
> + max_len = 30
> 
> as a method that is reused in both truncate() and
linked_entry_truncated_title()

I'll do that!

http://rietku.appspot.com/3001/diff/1/2#newcode114
Line 114: if len(value) >= max_len:
On 2009/03/27 20:37:58, adewale wrote:
> Can you add a test for this method. Especially the branch of the code that
deals
> with u"\u2026" as it's not obvious what that's trying to do.

Sure! The 2026 is taken from the original code. I was going to use …
(...) but assumed the original use of the utf-8 character was used for good
reason (saving number of bytes sent across the network?)

[lemonad]

Updated patch. Also, I had forgotten to include the new templates last time I
submitted.

[adewale]

http://rietku.appspot.com/3001/diff/1004/1005
File common/templatetags/format.py (right):

http://rietku.appspot.com/3001/diff/1004/1005#newcode110
Line 110: return 30
Why 30? Can we make this a constant?

[lemonad]

http://rietku.appspot.com/3001/diff/1004/1005
File common/templatetags/format.py (right):

http://rietku.appspot.com/3001/diff/1004/1005#newcode110
Line 110: return 30
On 2009/03/29 00:02:51, adewale wrote:
> Why 30? Can we make this a constant?

30 was the default for the truncate function (see -diff below). The actual value
used is defined in the templates which call truncate so changing this default
has currently no effect.

I think it might have been a mistake from my side to include 30 in the
max_len_or_default() since that gives the impression there's an agreement on the
default length when there's not.

What I mean is that we can likely agree on a default length when it comes to
truncated entry titles but on any issue where truncation should take place?
Probably not.

Making configurable options out of some of the hard coded values in the
templates is a good idea though. I'm not sure settings.py is the best place for
those options, what do you think? I would need something like this:

# None means no truncation
IM_MAX_LEN_OF_ENTRY_TITLES_FOR_COMMENTS_PLAIN = None
IM_MAX_LEN_OF_ENTRY_TITLES_FOR_COMMENTS_HTML = 40

[adewale]

On 2009/03/29 11:05:43, lemonad wrote:
> http://rietku.appspot.com/3001/diff/1004/1005
> File common/templatetags/format.py (right):
> 
> http://rietku.appspot.com/3001/diff/1004/1005#newcode110
> Line 110: return 30
> On 2009/03/29 00:02:51, adewale wrote:
> > Why 30? Can we make this a constant?
> 
> 30 was the default for the truncate function (see -diff below). The actual
value
> used is defined in the templates which call truncate so changing this default
> has currently no effect.
> 
> I think it might have been a mistake from my side to include 30 in the
> max_len_or_default() since that gives the impression there's an agreement on
the
> default length when there's not.
> 
> What I mean is that we can likely agree on a default length when it comes to
> truncated entry titles but on any issue where truncation should take place?
> Probably not.
> 
> Making configurable options out of some of the hard coded values in the
> templates is a good idea though. I'm not sure settings.py is the best place
for
> those options, what do you think? I would need something like this:
> 
> # None means no truncation
> IM_MAX_LEN_OF_ENTRY_TITLES_FOR_COMMENTS_PLAIN = None
> IM_MAX_LEN_OF_ENTRY_TITLES_FOR_COMMENTS_HTML = 40

I think that settings.py is an excellent place for these defaults. It makes it
easier for people to tweak their JaikuEngine instance from one place. Please go
ahead and make the change.

[termie]

quite excellent :) I would like to pass two arguments to send_message, plain
text and html, I think it is reasonable to assume that a common IM
implementation may choose not to allow raw xml over its api

[lemonad]

On 2009/04/08 02:40:54, termie wrote:
> quite excellent :) I would like to pass two arguments to send_message, plain
> text and html, I think it is reasonable to assume that a common IM
> implementation may choose not to allow raw xml over its api

I think I have (at least) two options;  one of which is to break up the template
into two. The other is to use a conditional in the template which
enables/disables html output.

Knowing little about the likely IM backend for end users, I think I'd prefer the
second option as that would enable easy extension of the actual contents of the
xmpp message.

In that sense, the more of the XMPP packet that could be moved to the template,
the better. I'm sure something is speaking against that though — please
enlighten me :)

[termie]

On 2009/04/08 05:55:24, lemonad wrote:
> On 2009/04/08 02:40:54, termie wrote:
> > quite excellent :) I would like to pass two arguments to send_message, plain
> > text and html, I think it is reasonable to assume that a common IM
> > implementation may choose not to allow raw xml over its api
> 
> I think I have (at least) two options;  one of which is to break up the
template
> into two. The other is to use a conditional in the template which
> enables/disables html output.
> 

how about im_comment.html and im_comment.txt for the two templates?

> Knowing little about the likely IM backend for end users, I think I'd prefer
the
> second option as that would enable easy extension of the actual contents of
the
> xmpp message.
> 
> In that sense, the more of the XMPP packet that could be moved to the
template,
> the better. I'm sure something is speaking against that though — please
> enlighten me :)

[adewale]

LGTM apart from the need to extract out a method

http://rietku.appspot.com/3001/diff/6001/7001
File common/templatetags/format.py (right):

http://rietku.appspot.com/3001/diff/6001/7001#newcode118
Line 118: try:
Can you extract these 4 lines into a method that is called both here and below

http://rietku.appspot.com/3001/diff/6001/7001#newcode158
Line 158: try:
Can you extract these 4 lines into a method that is called both here and above

[termie]

some small things, but otherwise looks great

http://rietku.appspot.com/3001/diff/6001/7002
File common/api.py (right):

http://rietku.appspot.com/3001/diff/6001/7002#newcode4416
Line 4416: xmpp_connection.send_message(im_aliases, plain_text_message,
split lines, please, so:

im_aliases,
plain_text_message,
html_message=html_message

http://rietku.appspot.com/3001/diff/6001/7002#newcode4446
Line 4446: xmpp_connection.send_message(im_aliases, plain_text_message,
same as above

http://rietku.appspot.com/3001/diff/6001/7003
File common/protocol/xmpp.py (right):

http://rietku.appspot.com/3001/diff/6001/7003#newcode77
Line 77: message, html_message)
one arg per line at this stage, please

http://rietku.appspot.com/3001/diff/6001/7005
File common/templates/im/im_entry.html (right):

http://rietku.appspot.com/3001/diff/6001/7005#newcode1
Line 1: {% spaceless %}{% load format i18n %}
if we're using spaceless, it would be nice to add some more space between things
for readability (applies to all the templates)

http://rietku.appspot.com/3001/diff/6001/7006
File common/templates/im/im_entry.txt (right):

http://rietku.appspot.com/3001/diff/6001/7006#newcode1
Line 1: {% spaceless %}{% load format i18n %}
what is spaceless accomplishing here?

[lemonad]

http://rietku.appspot.com/3001/diff/6001/7001
File common/templatetags/format.py (right):

http://rietku.appspot.com/3001/diff/6001/7001#newcode158
Line 158: try:
On 2009/04/10 21:04:10, adewale wrote:
> Can you extract these 4 lines into a method that is called both here and above

Just to make sure; do you still want me to break the code out into a separate
method even though the two instances are not identical?

[termie]

http://rietku.appspot.com/3001/diff/6001/7001
File common/templatetags/format.py (right):

http://rietku.appspot.com/3001/diff/6001/7001#newcode158
Line 158: try:
On 2009/04/10 21:33:35, lemonad wrote:
> On 2009/04/10 21:04:10, adewale wrote:
> > Can you extract these 4 lines into a method that is called both here and
above
> 
> Just to make sure; do you still want me to break the code out into a separate
> method even though the two instances are not identical?

i don't particularly feel that there is much to gain by extracting it, i'm of
the refactor after three copy-pastes but two is fine mentality

[termie]

On 2009/04/10 21:43:16, termie wrote:
> http://rietku.appspot.com/3001/diff/6001/7001
> File common/templatetags/format.py (right):
> 
> http://rietku.appspot.com/3001/diff/6001/7001#newcode158
> Line 158: try:
> On 2009/04/10 21:33:35, lemonad wrote:
> > On 2009/04/10 21:04:10, adewale wrote:
> > > Can you extract these 4 lines into a method that is called both here and
> above
> > 
> > Just to make sure; do you still want me to break the code out into a
separate
> > method even though the two instances are not identical?
> 
> i don't particularly feel that there is much to gain by extracting it, i'm of
> the refactor after three copy-pastes but two is fine mentality

LGTM, will merge and commit

[termie]

On 2009/04/10 23:02:23, termie wrote:
> On 2009/04/10 21:43:16, termie wrote:
> > http://rietku.appspot.com/3001/diff/6001/7001
> > File common/templatetags/format.py (right):
> > 
> > http://rietku.appspot.com/3001/diff/6001/7001#newcode158
> > Line 158: try:
> > On 2009/04/10 21:33:35, lemonad wrote:
> > > On 2009/04/10 21:04:10, adewale wrote:
> > > > Can you extract these 4 lines into a method that is called both here and
> > above
> > > 
> > > Just to make sure; do you still want me to break the code out into a
> separate
> > > method even though the two instances are not identical?
> > 
> > i don't particularly feel that there is much to gain by extracting it, i'm
of
> > the refactor after three copy-pastes but two is fine mentality
> 
> LGTM, will merge and commit

woops, looks like there is no common/test/format.py

[adewale]

Actually now that I think about it there's no easy way to extract out those 4
lines into a re-usable method.

Everything else is fine.

Looks Good To Me.