[reportlab-users] Hebrew Support Patch

[Marius Gedminas]

On Fri, Jun 05, 2009 at 10:02:57AM +0300, Moshe Wagner wrote:

> Thanks for your corrections.

> As I said, I'm new to python, so I didn't realize you could use loops

> that way.


Welcome.  I think you'll enjoy Python.  I do.


> This line:

> "if type(line) in (FragLine, ParaLines):"

>  didn't seem to work, as type(line) always returned "instance", so I

> kept it the way I had it.


Old-style classes FTW.

isinstance(line, (FragLine, ParaLines)) should work, and is better style
anyway.


> Another thing I did wrong is implementing '.reverse()' manually, not

> realizing it exists.

> 

> So here's the new code. Clearly looks better:

> 

> ######################

> # Hebrew text patch, Moshe Wagner, June 2009

> # <moshe.wagner at gmail.com>

> 

> #This code fixes paragraphs with RTL text

> 

> # It does it by flipping each line seperatly.

> #	(Depending on the type of the line)

> 

> # If fribidi cant be imported, it does nothing

> 

> try:

> 	import pyfribidi


Please don't use tabs for indentation.  4 spaces are traditional.  (The
Python coding style guidelines are almost universally accepted; you can
find them at http://www.python.org/dev/peps/pep-0008/)

Although last time I checked Reportlab didn't follow PEP-8 faithfully.


> 	for line in blPara.lines:	

> 		if line.__class__.__name__ in ('FragLine', 'ParaLines'):


isinstance(line, (FragLine, ParaLines))


> 			"""When the line is a FragLine or ParaLines, It's

> 			text attribute of each of it's words is flipped.

> 			Then, the order of the words is flipped too,

> 			So that 2 word parts on the same line

> 			will be in the right order """


Comments are written like this:

                        # When the line is a FragLine or ParaLines, It's
                        # text attribute of each of it's words is flipped.
                        # Then, the order of the words is flipped too,
                        # So that 2 word parts on the same line
                        # will be in the right order

Also, it's "its", not "It's".


> 			for word in line.words:

> 				word.text = pyfribidi.log2vis (word.text, base_direction = pyfribidi.ON)


PEP-8 recommends no space in front of ( following a function name, and
no spaces around = when used to pass keyword arguments.


> 				

> 			line.words.reverse()

> 

> 		elif line.__class__.__name__ == 'tuple':


isinstance(line, tuple)


> 			"""When the line is just a tuple whose second value is the text.

> 			since I coulden't directly change it's value,

> 			it's done by merging the words, flipping them,

> 			and re-entering them one by one to the second attribute """

> 			

> 			s = ' '.join(line[1])

> 			s = pyfribidi.log2vis( s, base_direction=pyfribidi.ON)

> 			line[1][:] = s.split()

> 		else:

> 			print line.__class__.__name__


This is for debugging purposes, right?


> 			#print self.blPara.lines[i][1][1:]

> except ImportError:

> 	print "Fribidi module not found; You will not have rtl support for

> this paragraph"


It's typically better to keep the part between try: and except: as small
as possible, thus it's idiomatic to write

    try:
        import pyfribidi
    except ImportError:
        print >> sys.stderr, "Fribidi module not found ..."
    else:
        # code

Printing to sys.stdout (or even sys.stderr) from a library is perhaps
bad style, but I'm not sure what's the right solution here.  Either the
logging or the warnings module would be better.  E.g.

    try:
        import pyfribidi
    except ImportError:
        warnings.warn("Fribidi module not found ...")
    else:
        # code


> ######################

> 

> Is there any chance this could be added to the official code?


I'm not affiliated with Reportlab in any way (other than being a happy
user and a some-time contributor a long time ago), but I'd say yes.  The
chances would be better if you added some tests for the new code.

Marius Gedminas
-- 
We have enough youth, how about a fountain of SMART?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : <http://two.pairlist.net/pipermail/reportlab-users/attachments/20090605/a5b14f24/attachment.pgp>