#5931 closed defect (worksforme)
[PATCH] compliance with Genshi's strict mode
| Reported by: | 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 )
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)
Change History (30)
comment:1 by , 16 years ago
| Keywords: | verify added |
|---|---|
| Milestone: | → 0.11.1 |
comment:2 by , 16 years ago
| Component: | wiki system → attachment |
|---|
comment:3 by , 14 years ago
| Keywords: | bitesized added |
|---|
comment:4 by , 14 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 , 14 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.
follow-up: 7 comment:6 by , 14 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?
comment:7 by , 14 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 , 14 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 , 14 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 , 14 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 , 14 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.
follow-up: 15 comment:10 by , 14 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 usepy:with="..."in a surrounding element or a <py:with vars="...">` element directive - are you sure the removal of
and not cnum_editcondition is OK? Especially that you setcnum_edittoFalsein some case a few lines above - to
change.cnum != False
preferchange.cnum is not False - gasp, even more
<?python ...?>blocks… thetry... 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 thatname == '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 withwiki_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 avoiddefined()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?
follow-up: 14 comment:11 by , 14 years ago
[OT] is it me, or are there two comment:9 in this ticket?!?
comment:12 by , 14 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 , 14 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.
comment:14 by , 14 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…
follow-up: 16 comment:15 by , 14 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.
follow-up: 17 comment:16 by , 14 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">
:-)
comment:17 by , 14 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 , 14 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 , 14 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
follow-up: 21 comment:20 by , 14 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).
comment:21 by , 14 years ago
| Component: | attachment → general |
|---|---|
| Milestone: | next-minor-0.12.x → 0.12 |
| Summary: | attachments.html template errors out under genshi strict mode → compliance 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 , 14 years ago
ok, i guess you don't need a new patch just for reverting this line. anything else?
comment:23 by , 14 years ago
| Keywords: | genshi added |
|---|---|
| Milestone: | 0.12 → next-minor-0.12.x |
Probably not for 0.12.
comment:24 by , 14 years ago
| Cc: | added |
|---|---|
| Summary: | compliance with Genshi's strict mode → {PATCH] compliance with Genshi's strict mode |
comment:25 by , 13 years ago
| Cc: | added; removed |
|---|---|
| Description: | modified (diff) |
comment:26 by , 11 years ago
| Resolution: | → worksforme |
|---|---|
| Status: | new → closed |
| 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 , 11 years ago
| Milestone: | next-minor-0.12.x |
|---|
We're now using the 'lenient' mode, but it might be interesting to check again what happens in strict mode.