Code review comment for lp:~brian-murray/apturl/python3-port

On Jun 11, 2012, at 08:41 PM, Brian Murray wrote:

>Brian Murray has proposed merging lp:~brian-murray/apturl/python3-port into lp:apturl.
>
>Requested reviews:
> Barry Warsaw (barry)
>
>For more details, see:
>https://code.launchpad.net/~brian-murray/apturl/python3-port/+merge/109730
>
>python3 port

=== modified file 'AptUrl/Helpers.py'
--- AptUrl/Helpers.py 2009-09-10 11:45:36 +0000
+++ AptUrl/Helpers.py 2012-06-11 20:40:26 +0000
> @@ -16,7 +16,7 @@
> # General Public License for more details.
> #
> # You should have received a copy of the GNU General Public License
> -# along with GDebi; if not, write to the Free Software
> +# along with AptUrl; if not, write to the Free Software
> # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>
> import gettext
> @@ -29,13 +29,14 @@
> return utf8(gettext.ngettext(singular, plural, n))
>
> def utf8(str):
> - if isinstance(str, unicode):
> + if isinstance(str, bytes):

Probably better:

    if str is bytes:

which will be true in Python 2 but not in Python 3.

=== modified file 'AptUrl/Parser.py'
--- AptUrl/Parser.py 2011-10-13 06:59:34 +0000
+++ AptUrl/Parser.py 2012-06-11 20:40:26 +0000
> @@ -105,7 +105,8 @@
> res = []
>
> if len(full_url) > MAX_URL_LEN:
> - url = "%s ..." % full_url[0:MAX_URL_LEN/10]

You can also use the // operator to restore integer division. This works in
both versions of Python.

> + url_cutoff = int(MAX_URL_LEN / 10)
> + url = "%s ..." % full_url[0:url_cutoff]
> raise InvalidUrlException(url, _("Url string '%s' too long") % url)
>
> # check against whitelist

=== modified file 'tests/apturlparse.py'
--- tests/apturlparse.py 2011-10-13 06:59:34 +0000
+++ tests/apturlparse.py 2012-06-11 20:40:26 +0000
=== modified file 'tests/test_helpers.py'
--- tests/test_helpers.py 2009-01-08 19:46:00 +0000
+++ tests/test_helpers.py 2012-06-11 20:40:26 +0000
> @@ -15,9 +15,9 @@
>
> def test_parse_pkg(self):
> pkgobj = MockPkg("summary\ndescr\n")
> - self.assert_(parse_pkg(pkgobj) == ('summary','descr\n',None))
> + self.assertTrue(parse_pkg(pkgobj) == ('summary','descr\n',None))
> pkgobj = MockPkg("summary only")
> - self.assert_(parse_pkg(pkgobj) == ('summary only','',None))
> + self.assertTrue(parse_pkg(pkgobj) == ('summary only','',None))

Why not change these to assertEqual() calls?

>
> if __name__ == "__main__":
> unittest.main()