Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(57)

Issue 29350236: Issue 4373 - Made jshydra compatible with Python 3 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 2 months ago by Sebastian Noack
Modified:
3 years, 2 months ago
Reviewers:
Vasily Kuznetsov
CC:
Jon Sonesen
Visibility:
Public.

Description

Issue 4373 - Made jshydra compatible with Python 3

Patch Set 1 : #

Total comments: 13

Patch Set 2 : Use urllib2, removed redundant future import, added option to test without caching #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+74 lines, -67 lines) Patch
M .gitignore View 1 chunk +2 lines, -1 line 0 comments Download
M .hgignore View 1 chunk +1 line, -0 lines 0 comments Download
M abp_rewrite.py View 1 1 chunk +9 lines, -23 lines 0 comments Download
M autotest.py View 2 chunks +21 lines, -21 lines 0 comments Download
A tox.ini View 1 1 chunk +13 lines, -0 lines 0 comments Download
M utils.py View 1 3 chunks +28 lines, -22 lines 2 comments Download

Messages

Total messages: 4
Sebastian Noack
https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py File abp_rewrite.py (left): https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py#oldcode1 abp_rewrite.py:1: #!/usr/bin/env python In all environments (except for a Python ...
3 years, 2 months ago (2016-08-28 22:36:14 UTC) #1
Vasily Kuznetsov
Looks good in general. What do you think about the proposal to add environments to ...
3 years, 2 months ago (2016-08-29 15:29:53 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py File abp_rewrite.py (right): https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py#newcode5 abp_rewrite.py:5: from __future__ import print_function I just noticed that this ...
3 years, 2 months ago (2016-08-30 11:44:39 UTC) #3
Vasily Kuznetsov
3 years, 2 months ago (2016-08-30 13:16:30 UTC) #4
LGTM

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py
File abp_rewrite.py (right):

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py#newc...
abp_rewrite.py:5: from __future__ import print_function
On 2016/08/30 11:44:38, Sebastian Noack wrote:
> I just noticed that this future import here isn't neccessary anymore. Since
the
> |__name__ == '__main__'| block has been removed, this module does not print
> anything anymore.

Acknowledged.

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py#newc...
abp_rewrite.py:13: def rewrite_js(args, script=None):
On 2016/08/28 22:36:13, Sebastian Noack wrote:
> Yes, this is a breaking change that needs to be adapted for when updating the
> dependecy in buildtools. The reason for this change was (besides migrating to
a
> PEP-8 conform name) was to avoid duplication in autotest.py and actually use
> this function there so that it gets tested as well.

Acknowledged.

https://codereview.adblockplus.org/29350236/diff/29350250/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29350236/diff/29350250/tox.ini#newcode6
tox.ini:6: commands = python autotest.py
On 2016/08/30 11:44:38, Sebastian Noack wrote:
> On 2016/08/29 15:29:52, Vasily Kuznetsov wrote:
> > On 2016/08/28 22:36:14, Sebastian Noack wrote:
> > > Note that in order to test all code paths, we'd have to make sure that
there
> > is
> > > no cached mozilla* directory, then run utils.ensureJSShell(), and then
make
> > sure
> > > it runs again. That would be easy enough here:
> > > 
> > >   commands =
> > >     rm -rf mozilla*
> > >     python -c 'import utils; utils.ensureJSShell()
> > >     python autotest.py
> > > 
> > > But then we'd always have to download jsshell (once per environment!) on
> every
> > > run. This would make the tests extremely slow, impossible to work offline,
> and
> > > causing unnecessary load on Mozilla's servers.
> > > 
> > > Theoretically, we could bundle a version of the jsshell package with the
> tests
> > > of course. However, the whole point of that code is to avoid bundling
> jsshell
> > > with our code.
> > > 
> > > So I guess, I will just leave it as it is.
> > 
> > What you say makes sense, however, downloading uses slightly different code
> > paths in Python2/3 so perhaps it would be good to add (not included by
> default)
> > environments that would do the `rm -rf ...` and a comment about why they are
> > needed. This way this knowledge will not only be in this review.
> 
> There you go, however, I decided to check for an optional argument, rather
than
> using separate environments. The main problem with using a separate
environment
> (besides being unnecessary) is that we want to test that code paths on all
> Python versions. That would be possible with a factor condition, but you would
> still have to explicitly specify all Python versions when running tox (i.e.
"tox
> -e py{27,35}-nocache").

Great, this is even better.

https://codereview.adblockplus.org/29350236/diff/29350304/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29350236/diff/29350304/utils.py#newcode13
utils.py:13: import urllib2
On 2016/08/30 11:44:39, Sebastian Noack wrote:
> It seems more resonable to fallback to urllib2 (rather than urllib) on Python
2,
> since it is closer to the semantics of urllib.request on Python 3, e.g. it
> raises when encountering bad HTTP responses.

Acknowledged.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5