Code review comment for lp:~rachidbm/ubuntu-l10n-tools/search

El dl 30 de 05 de 2011 a les 20:19 +0000, en/na Rachid va escriure:
> Hi David,
>
> On Mon, May 30, 2011 at 6:08 PM, David Planella
> <email address hidden>wrote:
>
> > Review: Needs Fixing
> > Hi Rachid,
> >
> > Thanks a lot for the branch!
> >
> > A couple of things I've noticed while reviewing it:
> >
> > * On setup.py you add "print >> sys.stderr, 'To build ul10n-tools you need
> > https://launchpad.net/python-distutils-extra : \nsudo apt-get install
> > python-distutils-extra python-polib'". While it is true, I don't think it's
> > the best way to check it here, as the exception only checks for
> > python-distutils-extra and shouldn't have anything to do with python-polib,
> > which is not a build dependency, but a runtime dependency. I think having
> > the check on the file that imports polib should be enough
> >
> OK, so I should only remove python-polib? Or the whole apt-get line? (I
> doubt because apt-get is Ubuntu/Debian specific)
>

I'd suggest removing the python-polib reference for now. I personally
wouldn't have put the apt-line there, since this makes the tool even
more Ubuntu-specific, but as we're just targetting Ubuntu for now, it
can stay if you want.

>
> >
> > * In ul10n_tools/lp_get_templates/__init__.py you add "DISTRO_CODENAMES =
> > ('hardy', 'intrepid', 'jaunty', 'karmic', 'lucid', 'maverick', 'natty')",
> > which is correct, but we should probably be more dynamic and use
> > launchpadlib to get the releases. Nothing to change here, just a mental note
> > to myself that we should check out the capabilites of the Launchpad API
> > concerning templates nowadays, to see if we can get rid of the
> > screen-scraping :)
> >
> I was thinking about handling the DISTRO_CODENAMES in a central place
> anyway. Didn't thought about the Launchpad API, but I shall investigate that
> later.
>

Yeah, good idea, perhaps just as a get_ubuntu_releases() function in
launchpadmanager.py that returns a list of active distro releases. IIRC
there is already some code in one of the tools that returns releases
from Launchpad (or at least the current release). Anyway, this was only
an idea, not directly related with this merge proposal.

>
> >
> > * This is optional but you are using correct constructs such as "Source
> > package: %s" % (options.source_package), but I'd recommend using "Source
> > package: {0}".format(options.source_package) (See the str.format method on
> > http://docs.python.org/library/stdtypes.html#string-methods: "This method
> > of string formatting is the new standard in Python 3.0, and should be
> > preferred to the % formatting [...]")
> >
> I'm not a seasoned Python programmer, thanks for this tip ;)
>
> >
> > * In the future, if you submit a branch for each feature (i.e. each change
> > you make for a tool), it will make it much easier to review (no need to do
> > it for this now, I'll just review the current branch).
> >
> OK
>
> >
> > * On ul10n_tools/lp_set_pot_priority/__init__.py and
> > ul10n_tools/search/TranslationsSearch.py you are adding some code that is
> > commented out. If it's not used, could you please remove it?
> >
> Which lines exactly? Do you mean the "Currently dead code" remark?
>

On ul10n_tools/lp_set_pot_priority/__init__.py, diff line 266, the whole
code that actually sets the priorities is commented out

+ """
+ #launchpad = Launchpad.login_with('Template priority setter',
launchpad_server)
[...]

On ul10n_tools/search/TranslationsSearch.py, diff line 289:

-import logging
-import polib
+#import logging
+#import polib

The polib line can go away, whereas logging should probably not be
commented out if we want to use logging to log errors and debug messages
in the module.

As per the dead code remark, I think it should go away too.
TranslationsSearch is a separate module, and the fact that we call it
and check for valid modes outside does not mean that it couldn't be used
by another script which does not check the mode, so in that case we do
need the check inside TranslationsSearch and it wouldn't be dead code
then.

>
> >
> > Do you think you could have a look at these points and re-submit the merge
> > proposal before merging?
> >
> > Thanks!
> > --
> > https://code.launchpad.net/~rachidbm/ubuntu-l10n-tools/search/+merge/62857
> > You are the owner of lp:~rachidbm/ubuntu-l10n-tools/search.
> >
>

--
David Planella
Ubuntu Translations Coordinator
www.ubuntu.com / www.davidplanella.wordpress.com
www.identi.ca/dplanella / www.twitter.com/dplanella