generator function is better python for CompareLocales.FileCollector

RESOLVED FIXED

Status

Mozilla Localizations
Infrastructure
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Pike, Assigned: Pike)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 293664 [details] [diff] [review]
use generator function instead of iterator object
Attachment #293664 - Flags: review?(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.
Attachment #293664 - Flags: review?(bhearsum) → review-
(Assignee)

Comment 3

11 years ago
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.
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.

Updated

11 years ago
Attachment #293664 - Flags: review- → review+
(Assignee)

Comment 5

11 years ago
Checked in.

Note from irc, python on MozillaBuild uses '\', sadly.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 6

11 years ago
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
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
(Assignee)

Comment 8

11 years ago
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

11 years ago
+      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
(Assignee)

Updated

10 years ago
Component: Testing → Infrastructure
Product: Core → Mozilla Localizations
QA Contact: testing → infrastructure
You need to log in before you can comment on or make changes to this bug.