Edgewall Software

Opened 16 years ago

Closed 16 years ago

#202 closed enhancement (fixed)

Markup should be interoperable with other stuff

Reported by: ianb@… Owned by: cmlenz
Priority: major Milestone: 0.5
Component: Template processing Version:
Keywords: Cc: ianb@…, ben@…

Description (last modified by cmlenz)

The Markup class can't be interoperable with other classes. Specifically in WebHelpers? we want to implement our own HTML literal class, basically equivalent to Markup('<tags>'). Our object has a method .__html__(), that both indicates the object is a literal, and can be used to give an HTML representation of the object.

To work with this, Markup.escape would have to test for this attribute. Right now it does an exact class check (even subclasses won't work). We can't even do any kind of conditional mix-in.

If the Markup class also provides an __html__ method, we could also support using Markup as a way of providing HTML literals in our own quoting code.

Also, Markup(0) == '', while Markup(1) == '1'.

Attachments (2)

ticket202.diff (2.2 KB) - added by cmlenz 16 years ago.
Patch
Element-__html__-r826+t211.diff (3.3 KB) - added by cboos 16 years ago.
Previous patch (fixed) + added Element.__html__ method. Applies on ticket:211:mapping-style-__mod__-only-and-no-Markup_new-r826.diff

Download all attachments as: .zip

Change History (10)

comment:1 Changed 16 years ago by cmlenz

  • Description modified (diff)

(description formatting fixes)

comment:2 in reply to: ↑ description Changed 16 years ago by cmlenz

  • Priority changed from blocker to major
  • Type changed from defect to enhancement

Replying to ianb@colorstudy.com:

If the Markup class also provides an __html__ method, we could also support using Markup as a way of providing HTML literals in our own quoting code.

So what exactly is that method to return? In the case of Markup, simply self (instance of unicode subclass), or unicode(self) (unicode object), or…?

Also, Markup(0) == '', while Markup(1) == '1'.

I can't reproduce this. Markup(0) is '0' here.

comment:3 Changed 16 years ago by ianb@…

So what exactly is that method to return? In the case of Markup, simply self (instance of unicode subclass), or unicode(self) (unicode object), or…?

It would return self, like:

class Markup:
    def __html__(self):
        return self

In escape it would look like:

def escape(thing):
    if hasattr(thing, '__html__'):
        return Markup(thing.__html__())
    ...

I can't reproduce this. Markup(0) is '0' here.

I was noticing a truth test in the code, where I would have expected text=='', but I didn't really test so I might have misread.

Changed 16 years ago by cmlenz

Patch

comment:4 Changed 16 years ago by cmlenz

  • Milestone set to 0.5
  • Status changed from new to assigned

I've attached a patch against trunk that implements this.

comment:5 Changed 16 years ago by cboos

In the patch for the _cspeedups.c,

59	        ret = PyObject_CallMethod(text, "__html__", NULL); 
60	        Py_INCREF(ret); 

the INCREF looks wrong here, as PyObject_CallMethod already returns a new reference.

comment:6 follow-up: Changed 16 years ago by cboos

I just came across a new use case for this: the genshi.Element itself could define this __html__ method, so that it becomes possible to pass tags in interpolated Markup strings.

Example in Trac:

 ...
    [(tag.p(_(Markup("Tickets for %(group)s:"),
              group=tag.a(query.group, ' ', v, href=href,
                          class_='query', title=title))),
 ...

See attachment:Element-__html__-r826+t211.diff, which contains attachment:ticket202.diff + the fix from comment:5. That patch applies on r826 + the latest patch in #211, which provides the mapping-style interpolation.

Note that without the comment:5 fix, I got crashes while running the tests.

The example snippet above was tested in Trac and it works fine with the above patch. To be fully functional with translations, I guess the Babel string extractor has to be enhanced as well in order to find the text in _(Markup(...)) expressions.

Changed 16 years ago by cboos

Previous patch (fixed) + added Element.__html__ method. Applies on ticket:211:mapping-style-__mod__-only-and-no-Markup_new-r826.diff

comment:7 in reply to: ↑ 6 Changed 16 years ago by cboos

Amending comment:6: After a discussion with cmlenz, it seems that we could as well handle that in Trac itself, with a new translation function dedicated to this case.

Example in Trac:

Could be rewritten:

  ...
     [(tag.p(M_("Tickets for %(group)s:",
                group=tag.a(query.group, ' ', v, href=href,
                            class_='query', title=title)),
  ...

The M_ is itself an alias for mgettext(msg, **kwargs) (for "Markup" gettext) and takes msg to be valid markup. The advantage of this approach is that we could even defer serialization by producing tags if needed (tag(Markup("Tickets for ", tag.a(...), Markup(":")) in the above example).

See attachment:Element-__html__-r826+t211.diff

So I'm not sure if it's still that much relevant now (don't forget to take into account the fix from comment:5 if you want to apply the original patch).

comment:8 Changed 16 years ago by cmlenz

  • Resolution set to fixed
  • Status changed from assigned to closed

Patch applied in [861].

Note: See TracTickets for help on using tickets.