Add line numbers to Python listings in documentation

[kehander1]

Line numbers in Python listings in the documentation would make it easier to refer to individual lines. It might be argued that such line numbers can easily go out of sync, but since the example listings don't get edited all that often, this does not strike me as much of a concern.

The attached change is a rather brazen kludge, though it does work. When spaces show up at the start of a line (in order to pad the line numbers), lore complains about indentation levels not matching - so this fix precedes the line number with a null character (ALT+255).

[kehander1]

Add line numbering to py-listing class

[exarkun]

Missing tests. Please don't use windows codepages for file encodings.

[exarkun]

Also:

1. don't use non-ascii in source files
2. including line numbers pre-parse makes all the source no longer valid python (as you noticed)
3. xml files can't include literal nuls

[kehander1]

Codeimport test files for lore\test

[kehander1]

This version adds line numbers after parse, and also checks to make sure that the numbers aren't being added in-between tags.

[exarkun]

Make sure to unassign the ticket so that people don't think you're going to review it.

[kehander1]

Add line numbering to py-listing class (v2)

[exarkun]

The test fails when I run it. Not really sure why. Since it's written in the conventional style of lore, it fails in a really obscure way that provides no real information about what's broken. :)

===============================================================================
[FAIL]: twisted.lore.test.test_lore.TestFactory.test_codeimport

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/twisted/lore/test/test_lore.py", line 340, in test_codeimport
    os.path.join(tmp, "lore_codeimport_test.html"))
  File "/home/exarkun/Projects/Twisted/trunk/twisted/lore/test/test_lore.py", line 69, in assertEqualFiles1
    self.assertEqualsFile(exp, fact.read())
  File "/home/exarkun/Projects/Twisted/trunk/twisted/lore/test/test_lore.py", line 78, in assertEqualsFile
    self.assertEqualsString(expected, act)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/lore/test/test_lore.py", line 82, in assertEqualsString
    self.assertEquals(len(expected), len(act))
twisted.trial.unittest.FailTest: not equal:
a = 7774
b = 7786

-------------------------------------------------------------------------------

It's unfortunate that most or all of the existing lore codebase serves primarily as an example of what not to do. :/

I recommend splitting the new logic out into a separate function and writing unit tests for that function. Don't write tests in the style of "process a big blob of xhtml and compare the output to a big blob of html". Instead, operate on individual units of functionality. Something needs to turn line number information into dom nodes. That's a unit to test. Something else needs to turn a dom object with line number information in it into some html. That's a unit to test. etc.

Oh, except that htmlizer doesn't operate on dom, it takes strings as input and produces strings as output. Atrocious. That means this change will be a lot harder to test. You can at least reduce the size of the blobs to under a hundred bytes and 2 or 3 lines, though. And there's no need to drop stuff onto the filesystem. This should work just as well with a StringIO as with an actual file.

Good luck. :)

[kehander1]

Line numbering function (v3)

[kehander1]

Makes perfect sense. I should have done it that way in the first place. Hopefully this new test isn't too simple.

As for why that last test failed, I imagine it had something to do with the path given for resources/stylesheet.css at the top of the html. (It didn't occur to me to use the latest SVN ; I was using the downloadable win32 package to run the test. Oops.)

[exarkun]

Thanks for the updated patch! This looks much better than the previous solution. Some new review feedback:

1. Test method docstrings of the form Test that X works. aren't as good as ones of the form X does y.. The latter form actually explains to a person what behavior is being tested, whereas the former just tells them something they already know - that they're reading a test method. For example, I would change the docstring of test_linenumbering to something like L{tree._addLineNumbers} accepts a C{str} and returns a C{str} which modifies each line in the input by prefixing it with a line number.
2. I might also name the test test_lineNumbering - convention is to capitalize second and subsequent words. It makes names easier to read.
3. _addLineNumbers uses 8 spaces for its first indentation, should be 4.
4. _addLineNumbers is missing a docstring
5. I'm not sure the strategy for numbering lines this code takes is good enough. There are a couple problems with it. They are minor, but maybe we can avoid them by generating some different markup.
   1. it's no longer possible to copy/paste code from listings - line numbers get included, making the code invalid
   2. the line numbers end up syntax colored

Having the line numbers and the source in two tds in a tr in a table might be a good approach (but I'm far from the premier expert on html and css so there could easily be a superior solution).

[kehander1]

In regards to the last point:

1. It's no worse than what Trac itself does. If you want to copy and paste from the actual code, the link to the unnumbered text is right there at the bottom of the listing. (The  PyGTK Tutorial is my direct inspiration, though of course I realize that what is good enough for PyGTK might not be good enough for here.)
2. Are you sure? I haven't seen that in my tests. That's the purpose of looking for a closing tag at the start of the line before inserting the number, since it seemed to me that the numbers only got hilighted when a newline got inserted before a closing tag. Do you have an example? If there are other cases when it might occur, perhaps it will be similarly possible to sweep them out of the way with a single additional check.

[exarkun]

The main coloring issue I saw was with multiline strings. I generated the twisted core docs with the patch applied (cd doc/core/howto; lore) and looked at some of the results.

[kehander1]

I looked through most of the documentation generated from doc/core/howto just now, and I still couldn't find any colored numbers. Is there a specific file where you saw them?

Maybe it's a weird platform-specific bug of some sort. I'm using Firefox 3.0.3 on Windows; how about you?

[exarkun]

Firefox 3.0.3 on Linux. The first file I looked in, application.html, had 4 or 5 green numbers.

[kehander1]

Now I see - My monitor's contrast settings are off.

[kehander1]

Line numbering function (v4)

[kehander1]

(Perhaps it would be a good idea to patch stylesheet.css so that strings are more brightly-colored?)

It was tricky, but I think this code solves the problem. Line numbers inside a multiline string now get enclosed in the appropriate tags so that they don't get hilighted. It might be easy to fool if you throw something really nonstandard at it (I tried to account for multiline strings that directly follow comments with no newline in between, for example), but I would think it unlikely for something of that nature to show up in the example listings.

[exarkun]

I'm really uncomfortable with the ad hoc html parsing in this patch. The line numbering is tightly coupled with the html generated by the htmlizer so it could easily be broken by changes to that code.

If we really want the line numbers to be in the html for displaying the source, then it'd be better if they were put there by the htmlizer. However, this approach still suffers from the copy/paste shortcoming I mentioned earlier. I'd still prefer a solution which addressed that as well.

It is indeed no worse than what trac does, but trac isn't the standard for what we do. ;) If there isn't any way to do better, then I'll live with this, but I'm not convinced that we can't do better yet.

If this implementation were at least a step toward something which didn't have this problem, then I'd consider accepting this approach and filing a ticket for enhancing it later (small changes are great, and incremental improvements are awesome). I can't see how this approach could be improved to support this use-case though, I think it needs to be rewritten in a completely different way.

Thanks for continuing to work on this.

[kehander1]

Line numbering function via float

[kehander1]

Very well... How about a floating label? At least the listing can be selected separately from the numbers. (I don't think the same thing can be done with tables.) This patch is a dreadfully inadequate proof-of-concept.

Inexperienced with CSS as I am, I have no idea how to make this work correctly without tables, if it can be done at all. Maybe someone else has a better idea. (I suspect throwing in single-pixel spacer images is probably not the approved method anymore.)

The topic of whether or not it matters whether the listing can be copied without the line numbers could probably be subjected to lengthy philosophical debate. Are the listings not there primarily so they can be read and examined in the page itself? Would the typical reader ever actually feel like copying and pasting individual segments of simple example code? Would said reader's needs not be easily met by clicking the link to the source listing?

[kehander1]

Line numbering function via table

[kehander1]

I was just looking at the way PyPi does things and it turns out you can do this with tables after all. Whoops. (I wonder why Trac doesn't do things that way?)

This still isn't quite right, but it's much closer.

[exarkun]

Looking much better. :) Think you can:

1. remove the assumption that one line of generated html equals one line of source
2. add unit tests

Thanks

[kehander1]

I'm rather more concerned about how the numbers still don't quite line up properly with the lines of code, and how the border at the bottom of the listing is all messed up. I can't figure out what the problem is.

[kehander1]

Now I see: rows that contain bold text have a different height than the other rows, throwing off the alignment. The example on PyPyi code.activestate.com that I was looking at has the same problem.

[kehander1]

Line numbering function via float with CSS edits

[kehander1]

There, that's more like it! It seems some minor CSS edits are necessary to make this work properly with Firefox 3.

If this still looks reasonable, I'll add the unit tests. (What was that about one line of generated html equaling one line of source? I don't understand how it could be otherwise.)

[kehander1]

Test for Line numbering function via float with CSS edits

[kehander1]

Here's the unit test anyway.

My only concern is that the CSS tags might be better placed in stylesheet.css rather than in every html page.

[exarkun]

This is looking pretty good. Some style comments:

• Why did you make the background for the line numbers a different color than the rest of the background?
• It'd be nice to have a bit more space between line numbers and the source line.
• Can we avoid having the line numbers overlap (and cover) the left border for the source box?

I definitely agree that putting the styling into the stylesheet would be better. This shouldn't be hard, right? Just give the p a new class and associate some styling with that class in stylesheet.css?

One other functional point - this is only being applied to nodes with the py-listing class. There's also the style of including Python code which involves a pre node with a python class. It seems like we'd want line numbers for those as well. Possibly the line-number adding stuff should be added to tree.py's fontifyPythonNode function and addPyListings should be made to use that function. Perhaps there's some reason that's not how it's already implemented, though (I haven't looked at the code carefully).

I still might want to have a go at a different Python implementation (that is, still emitting the html/css you've figured out, but in a different way) of this, so as to avoid having to split up html strings. That should be a minor point though, the hard part (figuring out what html/css to emit) you seem to have figured out. Thanks!

[kehander1]

Line numbering function via float with CSS edits (v3)

[kehander1]

I kind of liked the different color for the line numbers myself, but whatever. This ought to do it. Line numbers are now added to the python class nodes, with the exception of really short nodes.

I doubt it's impossible, but I can't really see an elegant way of rewriting fontifyPythonNode so that addPyListings could use it as well.

[exarkun]

(In [25524]) Branching to 'listing-line-numbers-3486'

[exarkun]

(In [25525]) Apply tree-linenum-float-css-v3.diff

refs #3486

[exarkun]

(In [25526]) do more things with DOM instead of strings

unfortunately, this is microdom and twisted.python.htmlizer is still a heap of crap, so it isn't a huge improvement, but it's a bit of an improvement perhaps

refs #3486

[exarkun]

Tweaked the implementation a bit. The output should still be the same.

[glyph]

I suggest adjusting the stylesheet to set a different background/foreground on the line numbers again. Other tools which do this (for a brief survey: gedit, trac). present the line numbers differently to make it clear that they're not part of the source text.

(It was annoying to review this because #3409 is still open and I had to experiment with a few different combinations of working-directory and command-line options before I could get something workable. I ended up with "lore" in "doc/core/howto". Since I'm not sure how the website is actually generated, my confidence that I've missed a functional regression isn't as high as I'd like.)

Otherwise I'm pretty happy with this branch, it is a huge improvement, as any modern edit of lore pretty much must be. Merge at your discretion, feel free to do the stylesheet adjustment separately.

[exarkun]

(In [25744]) make line number background a different color than source background

refs #3486

[exarkun]

(In [25745]) there is no extra padding anymore

refs #3486

[exarkun]

(In [25746]) Merge listing-line-numbers-3486

Author: kehander, exarkun Reviewer: exarkun, glyph Fixes: #3486

Add line numbering to Lore's Python source and listing markup support.