Edgewall Software

Ticket #202 (closed enhancement: fixed)

Opened 7 years ago

Last modified 7 years ago

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) (diff)

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

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

Change History

  Changed 7 years ago by cmlenz

  • description modified (diff)

(description formatting fixes)

in reply to: ↑ description   Changed 7 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.

  Changed 7 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 7 years ago by cmlenz

Patch

  Changed 7 years ago by cmlenz

  • status changed from new to assigned
  • milestone set to 0.5

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

  Changed 7 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.

follow-up: ↓ 7   Changed 7 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 Download, which contains attachment:ticket202.diff Download + 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 7 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 Download

in reply to: ↑ 6   Changed 7 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 Download

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).

  Changed 7 years ago by cmlenz

  • status changed from assigned to closed
  • resolution set to fixed

Patch applied in [861].

Add/Change #202 (Markup should be interoperable with other stuff)

Author


E-mail address and user name can be saved in the Preferences.


Change Properties
<Author field>
Action
as closed
The resolution will be deleted. Next status will be 'reopened'
 
Note: See TracTickets for help on using tickets.