408817 - generator function is better python for CompareLocales.FileCollector

Status: RESOLVED FIXED

(Mozilla Localizations :: Infrastructure, defect)

Description

[Axel Hecht [:Pike]]

Mozilla.CompareLocales.FileCollector is a beast right now, using a generator function rather than just implementing a iterator object is much nicer to the eyes.

Straight python-only thing, trying bhearsum for review on this one.

Comment 1

[Axel Hecht [:Pike]]

Attachment: use generator function instead of iterator object

Comment 2

[[e-mail] (:bhearsum)]

Comment on attachment 293664 [details] [diff] [review]
use generator function instead of iterator object

>+        leaf = dirpath + '/' + f

You should use os.path.join here. Other than, this looks fine. r=bhearsum with that change.

Comment 3

[Axel Hecht [:Pike]]

Actually, that'd regress the expressions in filter.py, those just check for '/'.

Note, I'm already ending up with forward slashes from the output of client.mk, the only code that inserts backslashes is os.walk, and I'd rather keep it that way. Or even work around that at one point.

Comment 4

[[e-mail] (:bhearsum)]

On unix platforms, os.path.join will insert a '/', right?
>>> a = "foo"
>>> b = "bar"
>>> import os.path
>>> os.path.join(a, b)
'foo/bar'

It sounds like this code is already unix specific, so I suppose it doesn't really matter that much.

Comment 5

[Axel Hecht [:Pike]]

Checked in.

Note from irc, python on MozillaBuild uses '\', sadly.

Comment 6

[Armen [:armenzg]]

Axel is good to see you got the bug already finished.
I am not that good with python, I understand very basic concepts but I get lost with things like:
- self.__w
- self.__i --- I guess it is where you assign an iterator
- self.__t --- it seems to have two arrays and some functionality, self.__t[1].sort()
A one character variable, doesn't give too much idea but I can get a feeling of what they do.
I will keep looking forward on learning python...

(In reply to comment #0)
> Mozilla.CompareLocales.FileCollector is a beast right now, using a generator
> function rather than just implementing a iterator object is much nicer to the
> eyes.
> 
> Straight python-only thing, trying bhearsum for review on this one.
> 
What do you mean by "generator function" rather than "implementing iterator"?
If too long to explain, don't bother I will find out eventually by seing more code

Comment 7

[[e-mail] (:bhearsum)]

Armen,

You should look at the Generator and Iterator design patterns. Here's a start:
http://en.wikipedia.org/wiki/Generator_(computer_science)
http://en.wikipedia.org/wiki/Iterator

Comment 8

[Axel Hecht [:Pike]]

Armen, if you look at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/testing/tests/l10n/lib/Mozilla/CompareLocales.py&rev=1.11&mark=56-68#47, does that still look confusing? I think not so, which is why I removed all the cryptic state variables stuff in favour of a generator.

The comparison of the two in the python docs is at http://docs.python.org/tut/node11.html#SECTION0011900000000000000000 (iterators), with the sexy stuff coming at http://docs.python.org/tut/node11.html#SECTION00111000000000000000000

Comment 9

[Armen [:armenzg]]

+      for f in filenames:
+        leaf = dirpath + '/' + f
+        yield (leaf[cutoff:], leaf)

Here is the yielding!!

I got it, I like generators!

Thanks both of you, I don't think I would have understood it properly if you didn't point where to look