Edgewall Software
Modify

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#9056 closed enhancement (fixed)

Refactor trac.mimeview.pygments.GenshiHtmlFormatter for clarity.

Reported by: me+trac@… Owned by: me+trac@…
Priority: normal Milestone: 0.12
Component: version control/browser Version: 0.12dev
Severity: minor Keywords: genshi mimeview highlighting
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

The generate() method currently does two things at once: it's chunking together tokens with the same class, and it's also outputting HTML tags. This patch separates them into two separate methods to improved clarity. Since both methods are just iterating over a generator, there's no additional memory overhead.

This does change code for the sake of just readability, but hopefully someone hacking on GenshiHtmlFormatter besides me will find it useful.

The patch is against today's trunk (r8899, source:trunk/trac/mimeview/pygments.py).

Attachments (1)

HtmlFormatter-rejiggle.diff (2.9 KB ) - added by me+trac@… 12 years ago.
Refactors GenshiHtmlFormatter.

Download all attachments as: .zip

Change History (6)

by me+trac@…, 12 years ago

Attachment: HtmlFormatter-rejiggle.diff added

Refactors GenshiHtmlFormatter.

comment:1 by Christian Boos, 12 years ago

This indeed looks prettier.

Seeing

if c == 'n': c = ''

makes me think you haven't read the TracDev/CodingStyle yet ;-)

Also, what is the speed impact of this change?

comment:2 by Christian Boos, 12 years ago

Milestone: 0.12

Tentatively set to 0.12, maybe someone could check the performance impact. If no loss, +1.

comment:3 by John Hampton, 12 years ago

Resolution: fixed
Status: newclosed

I ran some tests with ab against a local Trac instance rendering some python code. The performance was actually faster with the patch. Of course the difference was so small as to not be statistically significant.

Committed the patch with the minor change of

if c == 'n':
    c = ''

in [9269] since the patch has no performance impact.

Last edited 12 years ago by John Hampton (previous) (diff)

comment:4 by me+trac@…, 12 years ago

Thanks! Sorry I didn't have time to run the performance test sooner.

comment:5 by Christian Boos, 12 years ago

Owner: set to me+trac@…

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain me+trac@….
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from me+trac@… 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.