Edgewall Software
Modify

Opened 17 years ago

Closed 12 years ago

Last modified 12 years ago

#5931 closed defect (worksforme)

[PATCH] compliance with Genshi's strict mode

Reported by: ben@… Owned by: Christian Boos
Priority: normal Milestone:
Component: general Version:
Severity: normal Keywords: verify bitesized genshi
Cc: Thijs Triemstra Branch:
Release Notes:
API Changes:
Internal Changes:

Description (last modified by Thijs Triemstra)

I am using Trac 0.11dev-r5904 with Genshi 0.5dev-r726 and I get the following error any time I try to view an wiki page attachment:

File "/opt/local/lib/python2.5/site-packages/Trac-0.11dev_r5904-py2.5.egg/trac/templates/attachment.html", line 13, in <Suite u'parent = attachments and attachments.parent or context.parent; parent_name = parent.name(); parent_href = parent.get_href(href)'> <body py:with="parent = attachments and attachments.parent or context.parent;

This looks like the Genshi strict mode (default in Genshi 0.5) is breaking the attachments template.

System Information:

User Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en) AppleWebKit/522.11.1 (KHTML, like Gecko) Version/3.0.3 Safari/522.12.1

Trac: 0.11dev-r5904 Python: 2.5.1 (r251:54863, Jun 28 2007, 17:56:20) [GCC 4.0.1 (Apple Computer, Inc. build 5250)] setuptools: 0.6c5 SQLite: 3.4.0 pysqlite: 2.3.2 Genshi: 0.5dev-r726 Subversion: 1.4.2 (r22196) jQuery: 1.1.3.1

Attachments (2)

strict.patch (17.3 KB ) - added by Guy Rozendorn <guy@…> 15 years ago.
the next attachment is a patch that makes trac work under genshi strict mode — no errors in the browser, the code passes all the tests both under lenient and strict now. however, the patch does not change the mode back to strict — it leaves it on lenient as before.
strict.patch4 (16.7 KB ) - added by Guy Rozendorn <guy@…> 15 years ago.
second iteration of the patch

Download all attachments as: .zip

Change History (30)

comment:1 by Christian Boos, 17 years ago

Keywords: verify added
Milestone: 0.11.1

We're now using the 'lenient' mode, but it might be interesting to check again what happens in strict mode.

comment:2 by Piotr Kuczynski <piotr.kuczynski@…>, 16 years ago

Component: wiki systemattachment

comment:3 by Remy Blank, 15 years ago

Keywords: bitesized added

comment:4 by anonymous, 15 years ago

the attachment.html does break under 'strict' mode, but and that's not only thing that breaks under 'strict' mode. not sure you guys want to 'fix' this.

comment:5 by Remy Blank, 15 years ago

There weren't that many last time I checked, and this is an easy entry ticket for new contributors, hence the "bitesized" keyword.

comment:6 by Guy Rozendorn <guy@…>, 15 years ago

there were quite a few errors in ticket.html under 'strict', i *think* i fixed them all. i am now going over the attachment.html errors, and will look for others.

is there a test i can run to make sure I didn't break anything?

in reply to:  6 comment:7 by Remy Blank, 15 years ago

Replying to Guy Rozendorn <guy@…>:

is there a test i can run to make sure I didn't break anything?

Sure, see TracDev/UnitTests and TracDev/FunctionalTests.

comment:8 by Guy Rozendorn <guy@…>, 15 years ago

ran the functional tests against the 'strict' mode — it reported much more errors than are beneath the eye. i'll first work on making the webpages look OK under string, then fix all the errors from the tests.

comment:9 by Guy Rozendorn <guy@…>, 15 years ago

the next attachment is a patch that makes trac work under genshi strict mode — no errors in the browser, the code passes all the tests both under lenient and strict now. however, the patch does not change the mode back to strict — it leaves it on lenient as before.

comment:9 by Guy Rozendorn <guy@…>, 15 years ago

the next attachment is a patch that makes trac work under genshi strict mode — no errors in the browser, the code passes all the tests both under lenient and strict now. however, the patch does not change the mode back to strict — it leaves it on lenient as before.

by Guy Rozendorn <guy@…>, 15 years ago

Attachment: strict.patch added

the next attachment is a patch that makes trac work under genshi strict mode — no errors in the browser, the code passes all the tests both under lenient and strict now. however, the patch does not change the mode back to strict — it leaves it on lenient as before.

comment:10 by Christian Boos, 15 years ago

Thanks for working on this!

A few remarks:

  • we usually don't use <?python ... ?> PIs, or at least, not unless we're forced to; here the usual way would be to use py:with="..." in a surrounding element or a <py:with vars="...">` element directive
  • are you sure the removal of and not cnum_edit condition is OK? Especially that you set cnum_edit to False in some case a few lines above
  • to change.cnum != False
    prefer change.cnum is not False
  • gasp, even more <?python ...?> blocks… the try... except... if needed should really be done while preparing the data dictionary.
  • there's a field with idx 1 and a field that is None. why???
    isn't that name == 'id'?
  • to <py:choose test="'edit_label' in field"><py:when test="True">...<py:otherwise>...,
    prefer <py:choose test=""><py:when test="'edit_label' in field">...<py:otherwise>...

In general, when trying to make a template behave in strict mode, rather put the burden on the Python code than in the template. For example, in the following:

<py:if test="'optgroups' in field">
<optgroup py:for="optgroup in field.optgroups" 
    py:if="optgroup.options" 
    label="${optgroup.label}"> 
...
</optgroup> 
</py:if> 

Better make sure that the all field have an 'optgroups' member. The readability of the template matters, and the more we have logic there, the less readable it becomes (and this also my argument against the <?python ?> blocks, which you have to scrutinize to see if there's nothing fancy done there, whereas in <py:with> you can only have assignments).

Now continuing about readability:

   <image py:if="chrome.logo.src_abs"> 
   <image py:if="defined('chrome') and 'logo' in chrome and 'src_abs' in chrome.logo"> 

I start to wonder if it makes sense to convert everything to strict mode. Sure, it can catch the occasional error, but at the price of making the template a lot more obscure and complicated to write. Compare the straightforwardness of the chrome.logo.src_abs above with its strict counterpart.

  • wiki_to_html(..., repoinfo.description) replaced with wiki_to_html(..., repoinfo.name): no, the intent was not to wikify the repository name ;-)
  • trac/templates/diff_view.html: see comment above, about making sure the proper data is set in the Python code; it would make sense to add a helper function populating a dict with the appropriate entries (e.g. prepare_diff_view(data, url=...))
  • defined('merge') same thing, try to avoid defined() and make sure there's always an entry

I'm undecided on this topic now… what do others think? Are we sure the pay-offs of the strict mode are sufficient to justify less readable and more difficult to write templates?

comment:11 by Christian Boos, 15 years ago

[OT] is it me, or are there two comment:9 in this ticket?!?

comment:12 by ben@…, 15 years ago

FWIW, I (the original requestor) only care that Trac work with Genshi out of the box; if you think the best way to do that is to override Genshi's default settings rather than making Trac templates work in strict mode, that works for me.

comment:13 by Guy Rozendorn <guy@…>, 15 years ago

I think that you should work in strict mode, since there were lots of errors, in all sort of places. These errors are errors, and should not be ignored. I agree that putting 'fixes' in the templates is not the best way to resolve these errors, and it should be resolved one step earlier, where possible. For example, in ticket.html:242, under the forloop of 'change in changes', all changes should have a cnum. For another example, in ticket.html:182 — you access owner_link. well, not all tickets have owner_link, so I added a check to see if it is defined. I think the correct way to fix this is to set owner_link with a None default or a str() as default in the init or its equivalent.

Unless you guys want to neglect the 'strict' mode and live with the errors, I'll work on a smarter patch today.

in reply to:  11 comment:14 by Remy Blank, 15 years ago

Replying to cboos:

[OT] is it me, or are there two comment:9 in this ticket?!?

Yes, they were two separate submissions, 220818 microseconds apart. Which sounds to me like a double-click on "Submit changes", in combination with a race condition in the DB update code (between the part where it checks that the cnum is correct and the part where it actually saves the content). Ideally, this should all be done in a single transaction.

I guess that's the drawback of being able to do sub-second comments: you're now more likely to do sub-second comments…

in reply to:  10 ; comment:15 by Remy Blank, 15 years ago

Replying to cboos:

I'm undecided on this topic now… what do others think? Are we sure the pay-offs of the strict mode are sufficient to justify less readable and more difficult to write templates?

I'd say we should strive to be as close to strict-compliant as possible. As Guy mentioned, most of the issues are actual errors, and those should be fixed. I also think it's good practice anyway to make sure symbols are always defined when they are used, so they should be added to the template data, possibly with a value of None. My impression is that we can achieve this without sacrificing too much readability.

in reply to:  15 ; comment:16 by Christian Boos, 15 years ago

Replying to rblank:

… My impression is that we can achieve this without sacrificing too much readability.

Ok, so let's get a next iteration of the patch.

But really, I think the goal should be to still be able to write things like:

<image py:if="chrome.logo.src_abs"> 

in the template, instead of:

<image py:if="defined('chrome') and 'logo' in chrome and 'src_abs' in chrome.logo"> 

:-)

in reply to:  16 comment:17 by Remy Blank, 15 years ago

Replying to cboos:

But really, I think the goal should be to still be able to write things like:

Sure, that's what I meant with "not sacrificing readability". In this case, we should really make sure that chrome.logo.src_abs is always present, possibly empty or None.

comment:18 by Guy Rozendorn <guy@…>, 15 years ago

I agree. I am now working on a second iteration of the patch. I intend to pre-populate all the fields/attributes that are 'shaky' (that in the current trunk are something defined and sometimes are not) with a None value or empty dicts where needed. That way, if they get a 'real' value it'll be OK, and if not, then you could still use (for example) chrome.logo.src without checking if it is defined.

comment:19 by Guy Rozendorn <guy@…>, 15 years ago

Hello. I will upload in a moment the second iteration of my patch. it passes all tests (except "Test for regression of http://trac.edgewall.org/ticket/8247" which is broken in the trunk) in both strict and lenient mode, but i left it in lenient just in case

comment:20 by Guy Rozendorn <guy@…>, 15 years ago

Since this fix is for a much broader problem than attachments.html, i think it should be tracked in a new (or several new) ticket(s).

by Guy Rozendorn <guy@…>, 15 years ago

Attachment: strict.patch4 added

second iteration of the patch

in reply to:  20 comment:21 by Christian Boos, 15 years ago

Component: attachmentgeneral
Milestone: next-minor-0.12.x0.12
Summary: attachments.html template errors out under genshi strict modecompliance with Genshi's strict mode

Replying to Guy Rozendorn <guy@…>:

Since this fix is for a much broader problem than attachments.html, i think it should be tracked in a new (or several new) ticket(s).

No, no, it's fine here, we can refocus tickets when it makes sense, and here it does since most of the discussion is about Genshi strict mode.

Now the second iteration looks much better, but I still saw a glitch already mentioned in comment:10 (wikifying repo.name when it should be the repo.description).

comment:22 by Guy Rozendorn <guy@…>, 15 years ago

ok, i guess you don't need a new patch just for reverting this line. anything else?

comment:23 by Christian Boos, 15 years ago

Keywords: genshi added
Milestone: 0.12next-minor-0.12.x

Probably not for 0.12.

comment:24 by Thijs Triemstra <lists@…>, 14 years ago

Cc: lists@… added
Summary: compliance with Genshi's strict mode{PATCH] compliance with Genshi's strict mode

comment:25 by Thijs Triemstra, 14 years ago

Cc: Thijs Triemstra added; lists@… removed
Description: modified (diff)

comment:26 by Christian Boos, 12 years ago

Resolution: worksforme
Status: newclosed
Summary: {PATCH] compliance with Genshi's strict mode[PATCH] compliance with Genshi's strict mode

Well, Genshi "non-strict" mode is good enough for our needs, so we're not going to invest more time and energy on the conversion to "strict", at this point.

comment:27 by Christian Boos, 12 years ago

Milestone: next-minor-0.12.x

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.