Unified Diff: tox.ini
Issue 29501558:
Issue 5383 - Add tests for the Chrome and Firefox packagers (Closed)
Patch Set: Adding pytest-cov, adhering to comments
Created Aug. 14, 2017, 2:19 p.m.
Use n/p to move between diff chunks;
N/P to move between comments.
« tests/metadata.gecko ('K')
|
« tests/tools.py ('k')
|
no next file »
|
no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
| Index: tox.ini |
| diff --git a/tox.ini b/tox.ini |
| index 0083acc023bf9f7ad54e7a62d7c26cb0760da18c..0afbccc2392fec87158a9ddc17fa64950a801919 100644 |
| --- a/tox.ini |
| +++ b/tox.ini |
| @@ -19,7 +19,10 @@ putty-ignore = |
| setenv = |
| PYTHONPATH = {toxinidir}/.. |
| deps = |
| + pycrypto |
| + pillow |
|
Sebastian Noack
2017/08/14 15:20:12
The README (in adblockpluschrome) lists PIL (not P
The README (in adblockpluschrome) lists PIL (not Pillow) as a dependency:
https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
Pillow is meant to be a compatible drop-in replacement for PIL. So in theory
buildtools should work with both, and since in some environments only either is
available, it would be a good idea to ensure compatiblity with PIL and Pillow:
[tox]
envlist=py27-{PIL,pillow}
[testenv]
deps=
PIL: PIL
pillow: pillow
...
tlucas
2017/09/06 09:04:52
Got rid of PIL / pillow dependency completely (by
On 2017/08/14 15:20:12, Sebastian Noack wrote:
> The README (in adblockpluschrome) lists PIL (not Pillow) as a dependency:
> https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
>
> Pillow is meant to be a compatible drop-in replacement for PIL. So in theory
> buildtools should work with both, and since in some environments only either
is
> available, it would be a good idea to ensure compatiblity with PIL and Pillow:
>
> [tox]
> envlist=py27-{PIL,pillow}
>
> [testenv]
> deps=
> PIL: PIL
> pillow: pillow
> ...
Got rid of PIL / pillow dependency completely (by adding pre-generated images)
Sebastian Noack
2017/09/06 18:00:00
Well, the make_icons() function in packagerChrome.
On 2017/09/06 09:04:52, tlucas wrote:
> On 2017/08/14 15:20:12, Sebastian Noack wrote:
> > The README (in adblockpluschrome) lists PIL (not Pillow) as a dependency:
> > https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
> >
> > Pillow is meant to be a compatible drop-in replacement for PIL. So in theory
> > buildtools should work with both, and since in some environments only either
> is
> > available, it would be a good idea to ensure compatiblity with PIL and
Pillow:
> >
> > [tox]
> > envlist=py27-{PIL,pillow}
> >
> > [testenv]
> > deps=
> > PIL: PIL
> > pillow: pillow
> > ...
>
> Got rid of PIL / pillow dependency completely (by adding pre-generated images)
Well, the make_icons() function in packagerChrome.py is using PIL/Pillow as
well. But since this seems to be the only use of PIL/Pillow, we still have, and
getting the size of a PNG image is simple enough without an image processing
library, we might want to just do that and get rid of that dependency.
https://stackoverflow.com/questions/8032642/how-to-obtain-image-size-using-st...
Vasily Kuznetsov
2017/09/07 20:27:50
Guys! Writing your own image format parser is rath
On 2017/09/06 18:00:00, Sebastian Noack wrote:
> On 2017/09/06 09:04:52, tlucas wrote:
> > On 2017/08/14 15:20:12, Sebastian Noack wrote:
> > > The README (in adblockpluschrome) lists PIL (not Pillow) as a dependency:
> > >
https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
> > >
> > > Pillow is meant to be a compatible drop-in replacement for PIL. So in
theory
> > > buildtools should work with both, and since in some environments only
either
> > is
> > > available, it would be a good idea to ensure compatiblity with PIL and
> Pillow:
> > >
> > > [tox]
> > > envlist=py27-{PIL,pillow}
> > >
> > > [testenv]
> > > deps=
> > > PIL: PIL
> > > pillow: pillow
> > > ...
> >
> > Got rid of PIL / pillow dependency completely (by adding pre-generated
images)
>
> Well, the make_icons() function in packagerChrome.py is using PIL/Pillow as
> well. But since this seems to be the only use of PIL/Pillow, we still have,
and
> getting the size of a PNG image is simple enough without an image processing
> library, we might want to just do that and get rid of that dependency.
>
>
https://stackoverflow.com/questions/8032642/how-to-obtain-image-size-using-st...
Guys! Writing your own image format parser is rather radical NIHilism, given
that Pillow library is available and is quite stable and standard. However, if
it's only about PNG, it is not too complicated indeed, so be my guests, knock
yourselves out ;)
Sebastian Noack
2017/09/07 20:39:00
Well, all we need at the current point, is extract
On 2017/09/07 20:27:50, Vasily Kuznetsov wrote:
> On 2017/09/06 18:00:00, Sebastian Noack wrote:
> > On 2017/09/06 09:04:52, tlucas wrote:
> > > On 2017/08/14 15:20:12, Sebastian Noack wrote:
> > > > The README (in adblockpluschrome) lists PIL (not Pillow) as a
dependency:
> > > >
> https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
> > > >
> > > > Pillow is meant to be a compatible drop-in replacement for PIL. So in
> theory
> > > > buildtools should work with both, and since in some environments only
> either
> > > is
> > > > available, it would be a good idea to ensure compatiblity with PIL and
> > Pillow:
> > > >
> > > > [tox]
> > > > envlist=py27-{PIL,pillow}
> > > >
> > > > [testenv]
> > > > deps=
> > > > PIL: PIL
> > > > pillow: pillow
> > > > ...
> > >
> > > Got rid of PIL / pillow dependency completely (by adding pre-generated
> images)
> >
> > Well, the make_icons() function in packagerChrome.py is using PIL/Pillow as
> > well. But since this seems to be the only use of PIL/Pillow, we still have,
> and
> > getting the size of a PNG image is simple enough without an image processing
> > library, we might want to just do that and get rid of that dependency.
> >
> >
>
https://stackoverflow.com/questions/8032642/how-to-obtain-image-size-using-st...
>
> Guys! Writing your own image format parser is rather radical NIHilism, given
> that Pillow library is available and is quite stable and standard. However, if
> it's only about PNG, it is not too complicated indeed, so be my guests, knock
> yourselves out ;)
Well, all we need at the current point, is extracting the image size of an PNG
image. This is a trivial thing to do by parsing the relevant bytes from the PNG
header, and would avoid having us to instruct users to install either PIL or
Pillow, and testing our code with both libraries.
Sebastian Noack
2017/09/08 21:07:12
It is related to the tests, as it simplifies the t
> On 2017/09/07 20:39:00, Sebastian Noack wrote:
> > https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini
> > File tox.ini (right):
> >
> > https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini#newcode23
> > tox.ini:23: pillow
> > On 2017/09/07 20:27:50, Vasily Kuznetsov wrote:
> > > On 2017/09/06 18:00:00, Sebastian Noack wrote:
> > > > On 2017/09/06 09:04:52, tlucas wrote:
> > > > > On 2017/08/14 15:20:12, Sebastian Noack wrote:
> > > > > > The README (in adblockpluschrome) lists PIL (not Pillow) as a
> > dependency:
> > > > > >
> > >
https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
> > > > > >
> > > > > > Pillow is meant to be a compatible drop-in replacement for PIL. So
in
> > > theory
> > > > > > buildtools should work with both, and since in some environments
only
> > > either
> > > > > is
> > > > > > available, it would be a good idea to ensure compatiblity with PIL
and
> > > > Pillow:
> > > > > >
> > > > > > [tox]
> > > > > > envlist=py27-{PIL,pillow}
> > > > > >
> > > > > > [testenv]
> > > > > > deps=
> > > > > > PIL: PIL
> > > > > > pillow: pillow
> > > > > > ...
> > > > >
> > > > > Got rid of PIL / pillow dependency completely (by adding pre-generated
> > > images)
> > > >
> > > > Well, the make_icons() function in packagerChrome.py is using PIL/Pillow
> as
> > > > well. But since this seems to be the only use of PIL/Pillow, we still
> have,
> > > and
> > > > getting the size of a PNG image is simple enough without an image
> processing
> > > > library, we might want to just do that and get rid of that dependency.
> > > >
> > > >
> > >
> >
>
https://stackoverflow.com/questions/8032642/how-to-obtain-image-size-using-st...
> > >
> > > Guys! Writing your own image format parser is rather radical NIHilism,
given
> > > that Pillow library is available and is quite stable and standard.
However,
> if
> > > it's only about PNG, it is not too complicated indeed, so be my guests,
> knock
> > > yourselves out ;)
> >
> > Well, all we need at the current point, is extracting the image size of an
PNG
> > image. This is a trivial thing to do by parsing the relevant bytes from the
> PNG
> > header, and would avoid having us to instruct users to install either PIL or
> > Pillow, and testing our code with both libraries.
>
> I don't have a too strong opinion about this (i tend to not want to
reimplement
> this) - but either way, i guess it should be a separate issue, since it does
not
> relate to the tests
It is related to the tests, as it simplifies the tests. If we keep using
PIL/Pillow, my initial comment in this thread is still valid. But in order to
make sure everything works with both PIL and Pillow, we wouldn't only have to
add a couple additional lines to tox.ini, but also cause the tests to take twice
as long to run. Not to mention an additional dependency for users of buildtools.
Also note that detecting the size of a PNG image takes the same amount of code,
with PIL/Pillow than it does without:
width, height = struct.unpack('>ii', files[filename][16:24])
width, height = Image.open(StringIO(files[filename])).size
Anyway, if you still prefer to use PIL/Pillow, make at least sure that the code
is tested with both libraries.
Vasily Kuznetsov
2017/09/11 12:50:06
Seems like your proposal would work fine and it's
On 2017/09/08 21:07:12, Sebastian Noack wrote:
> > On 2017/09/07 20:39:00, Sebastian Noack wrote:
> > > https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini
> > > File tox.ini (right):
> > >
> > >
https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini#newcode23
> > > tox.ini:23: pillow
> > > On 2017/09/07 20:27:50, Vasily Kuznetsov wrote:
> > > > On 2017/09/06 18:00:00, Sebastian Noack wrote:
> > > > > On 2017/09/06 09:04:52, tlucas wrote:
> > > > > > On 2017/08/14 15:20:12, Sebastian Noack wrote:
> > > > > > > The README (in adblockpluschrome) lists PIL (not Pillow) as a
> > > dependency:
> > > > > > >
> > > >
> https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
> > > > > > >
> > > > > > > Pillow is meant to be a compatible drop-in replacement for PIL. So
> in
> > > > theory
> > > > > > > buildtools should work with both, and since in some environments
> only
> > > > either
> > > > > > is
> > > > > > > available, it would be a good idea to ensure compatiblity with PIL
> and
> > > > > Pillow:
> > > > > > >
> > > > > > > [tox]
> > > > > > > envlist=py27-{PIL,pillow}
> > > > > > >
> > > > > > > [testenv]
> > > > > > > deps=
> > > > > > > PIL: PIL
> > > > > > > pillow: pillow
> > > > > > > ...
> > > > > >
> > > > > > Got rid of PIL / pillow dependency completely (by adding
pre-generated
> > > > images)
> > > > >
> > > > > Well, the make_icons() function in packagerChrome.py is using
PIL/Pillow
> > as
> > > > > well. But since this seems to be the only use of PIL/Pillow, we still
> > have,
> > > > and
> > > > > getting the size of a PNG image is simple enough without an image
> > processing
> > > > > library, we might want to just do that and get rid of that dependency.
> > > > >
> > > > >
> > > >
> > >
> >
>
https://stackoverflow.com/questions/8032642/how-to-obtain-image-size-using-st...
> > > >
> > > > Guys! Writing your own image format parser is rather radical NIHilism,
> given
> > > > that Pillow library is available and is quite stable and standard.
> However,
> > if
> > > > it's only about PNG, it is not too complicated indeed, so be my guests,
> > knock
> > > > yourselves out ;)
> > >
> > > Well, all we need at the current point, is extracting the image size of an
> PNG
> > > image. This is a trivial thing to do by parsing the relevant bytes from
the
> > PNG
> > > header, and would avoid having us to instruct users to install either PIL
or
> > > Pillow, and testing our code with both libraries.
> >
> > I don't have a too strong opinion about this (i tend to not want to
> reimplement
> > this) - but either way, i guess it should be a separate issue, since it does
> not
> > relate to the tests
>
> It is related to the tests, as it simplifies the tests. If we keep using
> PIL/Pillow, my initial comment in this thread is still valid. But in order to
> make sure everything works with both PIL and Pillow, we wouldn't only have to
> add a couple additional lines to tox.ini, but also cause the tests to take
twice
> as long to run. Not to mention an additional dependency for users of
buildtools.
>
> Also note that detecting the size of a PNG image takes the same amount of
code,
> with PIL/Pillow than it does without:
>
> width, height = struct.unpack('>ii', files[filename][16:24])
>
> width, height = Image.open(StringIO(files[filename])).size
>
> Anyway, if you still prefer to use PIL/Pillow, make at least sure that the
code
> is tested with both libraries.
Seems like your proposal would work fine and it's simple and compact indeed, so
I don't mind. And eliminating dependency on PIL/Pillow is a good thing!
|
| pytest |
| + pytest-cov |
| jinja2 |
| # flake8-putty is not yet compatible with flake8 3.0 |
| flake8==2.* |
| @@ -27,5 +30,5 @@ deps = |
| pep8-naming |
| hg+https://hg.adblockplus.org/codingtools#egg=flake8-eyeo&subdirectory=flake8-eyeo |
| commands = |
| - py.test tests |
| + pytest tests --capture=sys --cov=buildtools --cov-report=term-missing |
| flake8 |
« tests/metadata.gecko ('K')
|
« tests/tools.py ('k')
|
no next file »
|
no next file with comments »
