Issue 29968569: Issue 4014 - Publish python-abp on PyPI (Closed)
|
Created:
Dec. 27, 2018, 11:31 p.m. by rhowell Modified:
Jan. 8, 2019, 10:33 p.m. Base URL:
https://hg.adblockplus.org/python-abp/ Visibility:
Public. |
DescriptionIssue 4014 - Publish python-abp on PyPI
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address comments on PS1, update README #
Total comments: 3
Patch Set 3 : Address comments on PS2, add README ToC #Patch Set 4 : Add MANIFEST.in #
Total comments: 13
Patch Set 5 : Address comment on PS4 #
Total comments: 5
Patch Set 6 : Address comment on PS5 #
MessagesTotal messages: 22
There were a couple questions I have: In setup.py, should we have a docstring for the public module and the public methods? setup.py:74 'Operating System :: OS Independent', # [Is this true?]
On 2018/12/27 23:33:02, rhowell wrote: > There were a couple questions I have: > In setup.py, should we have a docstring for the public module and the public > methods? Nah, it's just a build script. > setup.py:74 'Operating System :: OS Independent', # [Is this true?] It should, as we don't use any native code, neither rely on any os-dependent modules. But of course there is no way to know for sure unless you tested it on several operating systems. I would leave it in. https://codereview.adblockplus.org/29968569/diff/29968570/.coveragerc File .coveragerc (right): https://codereview.adblockplus.org/29968569/diff/29968570/.coveragerc#newcode3 .coveragerc:3: pragma: no cover It doesn't seem that we use this. Also it seems questionable in the first place to exclude lines from the coverage report regardless of the environment. https://codereview.adblockplus.org/29968569/diff/29968570/LICENSE File LICENSE (right): https://codereview.adblockplus.org/29968569/diff/29968570/LICENSE#newcode1 LICENSE:1: This file is part of Adblock Plus <https://adblockplus.org/>, This is just the disclaimer that should go at the top of every source file. If you include a LICENSE file it is supposed to be a copy of the whole license. For reference: https://www.gnu.org/licenses/gpl-howto.en.html https://codereview.adblockplus.org/29968569/diff/29968570/abp/filters/sources.py File abp/filters/sources.py (right): https://codereview.adblockplus.org/29968569/diff/29968570/abp/filters/sources... abp/filters/sources.py:184: raise err Previously, when we got an HTTPError with a code other than 404, this error was re-raised. Now, it will be silently ignored. https://codereview.adblockplus.org/29968569/diff/29968570/setup.py File setup.py (left): https://codereview.adblockplus.org/29968569/diff/29968570/setup.py#oldcode67 setup.py:67: 'Programming Language :: Python :: 2', Why did you remove this? This classifier indicates that it is compatible with any (not all) 2.x versions of Python. https://codereview.adblockplus.org/29968569/diff/29968570/setup.py#oldcode72 setup.py:72: ], You might want to add: Topic :: Software Development :: Libraries :: Python Modules https://codereview.adblockplus.org/29968569/diff/29968570/setup.py File setup.py (right): https://codereview.adblockplus.org/29968569/diff/29968570/setup.py#newcode20 setup.py:20: with open('README.md', 'r') as fh: This assumes that the README.md exists in the current working directory (rather than the directory setup.py is located in). open(os.path.join(os.path.dirname(__file__), 'README.md')) https://codereview.adblockplus.org/29968569/diff/29968570/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29968569/diff/29968570/tox.ini#newcode27 tox.ini:27: [testenv:coverage3] What's the point of using a separate environment for coverage, if you want to run coverage for both Python 2 and 3 anyway?
Thanks for the feedback! I've also updated the README, and found that the ValueError wasn't necessary at the bottom of sources.py. Let me know if you see any other issues? :) https://codereview.adblockplus.org/29968569/diff/29968570/.coveragerc File .coveragerc (right): https://codereview.adblockplus.org/29968569/diff/29968570/.coveragerc#newcode3 .coveragerc:3: pragma: no cover On 2018/12/28 00:22:51, Sebastian Noack wrote: > It doesn't seem that we use this. Also it seems questionable in the first place > to exclude lines from the coverage report regardless of the environment. Done. https://codereview.adblockplus.org/29968569/diff/29968570/LICENSE File LICENSE (right): https://codereview.adblockplus.org/29968569/diff/29968570/LICENSE#newcode1 LICENSE:1: This file is part of Adblock Plus <https://adblockplus.org/>, On 2018/12/28 00:22:51, Sebastian Noack wrote: > This is just the disclaimer that should go at the top of every source file. If > you include a LICENSE file it is supposed to be a copy of the whole license. > > For reference: https://www.gnu.org/licenses/gpl-howto.en.html Done. https://codereview.adblockplus.org/29968569/diff/29968570/abp/filters/sources.py File abp/filters/sources.py (right): https://codereview.adblockplus.org/29968569/diff/29968570/abp/filters/sources... abp/filters/sources.py:184: raise err On 2018/12/28 00:22:51, Sebastian Noack wrote: > Previously, when we got an HTTPError with a code other than 404, this error was > re-raised. Now, it will be silently ignored. Ah, you're right, thanks. https://codereview.adblockplus.org/29968569/diff/29968570/setup.py File setup.py (left): https://codereview.adblockplus.org/29968569/diff/29968570/setup.py#oldcode67 setup.py:67: 'Programming Language :: Python :: 2', On 2018/12/28 00:22:51, Sebastian Noack wrote: > Why did you remove this? This classifier indicates that it is compatible with > any (not all) 2.x versions of Python. Oops, guess I assumed it referred to Python 2.0. Fixed. https://codereview.adblockplus.org/29968569/diff/29968570/setup.py#oldcode72 setup.py:72: ], On 2018/12/28 00:22:51, Sebastian Noack wrote: > You might want to add: > > Topic :: Software Development :: Libraries :: Python Modules Done. https://codereview.adblockplus.org/29968569/diff/29968570/setup.py File setup.py (right): https://codereview.adblockplus.org/29968569/diff/29968570/setup.py#newcode20 setup.py:20: with open('README.md', 'r') as fh: On 2018/12/28 00:22:51, Sebastian Noack wrote: > This assumes that the README.md exists in the current working directory (rather > than the directory setup.py is located in). > > open(os.path.join(os.path.dirname(__file__), 'README.md')) Done. https://codereview.adblockplus.org/29968569/diff/29968570/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29968569/diff/29968570/tox.ini#newcode27 tox.ini:27: [testenv:coverage3] On 2018/12/28 00:22:51, Sebastian Noack wrote: > What's the point of using a separate environment for coverage, if you want to > run coverage for both Python 2 and 3 anyway? By default, tox only checks coverage for the version of Python that is the current default on the local system. Seems like it would be best to check both Python 2 & 3. But, I ran into an error trying to change the basepython variable in one environment: py._vendored_packages.iniconfig.ParseError: tox.ini:26: duplicate name 'basepython' This was a fairly quick & dirty work-around. I did some research, but wasn't able to find a better way yet, though this article was very informative: https://anonbadger.wordpress.com/2017/11/24/better-unittest-coverage-stats-ex... I'm open for discussion or suggestions on this, though.
https://codereview.adblockplus.org/29968569/diff/29968570/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29968569/diff/29968570/tox.ini#newcode27 tox.ini:27: [testenv:coverage3] On 2018/12/28 21:50:08, rhowell wrote: > On 2018/12/28 00:22:51, Sebastian Noack wrote: > > What's the point of using a separate environment for coverage, if you want to > > run coverage for both Python 2 and 3 anyway? > > By default, tox only checks coverage for the version of Python that is the > current default on the local system. Seems like it would be best to check both > Python 2 & 3. But, I ran into an error trying to change the basepython variable > in one environment: > > py._vendored_packages.iniconfig.ParseError: tox.ini:26: duplicate name > 'basepython' > > This was a fairly quick & dirty work-around. I did some research, but wasn't > able to find a better way yet, though this article was very informative: > https://anonbadger.wordpress.com/2017/11/24/better-unittest-coverage-stats-ex... > > I'm open for discussion or suggestions on this, though. If you just run coverage reports for every environment you run tests on, you don't have to explicitly set the basepython setting. However, the tricky part is to get the Python major version into the .coveragerc. This should do the trick: [testenv] deps = pytest-cov mock commands = python -c 'import sys; open(".coveragerc", "w").write("[report]\nexclude_lines = pragma: no py{} cover".format(sys.version_info[0]))' py.test --cov=abp --cov-report=term-missing tests https://codereview.adblockplus.org/29968569/diff/29969565/LICENSE File LICENSE (right): https://codereview.adblockplus.org/29968569/diff/29969565/LICENSE#newcode1 LICENSE:1: GNU LESSER GENERAL PUBLIC LICENSE Wrong license. This is LGPL, not the GPL.
You might also want to use the --cov-fail-under option to make sure coverage stays at 100%.
https://codereview.adblockplus.org/29968569/diff/29969565/LICENSE File LICENSE (right): https://codereview.adblockplus.org/29968569/diff/29969565/LICENSE#newcode1 LICENSE:1: GNU LESSER GENERAL PUBLIC LICENSE On 2018/12/28 22:39:02, Sebastian Noack wrote: > Wrong license. This is LGPL, not the GPL. The correct license has already been added, see the COPYING file. So adding this file is redundant in the first place. However, if you want the license to be included in your source distributions, you have to add a MANIFEST.in file with following contents (similar if you'd call the file LICENSE): include COPYING Some projects also include their tests in their source distributions. If you want to do likewise, your MANIFEST.in, should look like this: include COPYING tox.ini graft tests
Thanks for the input! Tox now runs coverage checks for all versions of python. I also added a table of contents to the README. Let me know if you see anything else? https://codereview.adblockplus.org/29968569/diff/29968570/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29968569/diff/29968570/tox.ini#newcode27 tox.ini:27: [testenv:coverage3] On 2018/12/28 22:39:02, Sebastian Noack wrote: > On 2018/12/28 21:50:08, rhowell wrote: > > On 2018/12/28 00:22:51, Sebastian Noack wrote: > > > What's the point of using a separate environment for coverage, if you want > to > > > run coverage for both Python 2 and 3 anyway? > > > > By default, tox only checks coverage for the version of Python that is the > > current default on the local system. Seems like it would be best to check both > > Python 2 & 3. But, I ran into an error trying to change the basepython > variable > > in one environment: > > > > py._vendored_packages.iniconfig.ParseError: tox.ini:26: duplicate name > > 'basepython' > > > > This was a fairly quick & dirty work-around. I did some research, but wasn't > > able to find a better way yet, though this article was very informative: > > > https://anonbadger.wordpress.com/2017/11/24/better-unittest-coverage-stats-ex... > > > > I'm open for discussion or suggestions on this, though. > > If you just run coverage reports for every environment you run tests on, you > don't have to explicitly set the basepython setting. However, the tricky part is > to get the Python major version into the .coveragerc. This should do the trick: > > [testenv] > deps = > pytest-cov > mock > commands = > python -c 'import sys; open(".coveragerc", > "w").write("[report]\nexclude_lines = pragma: no py{} > cover".format(sys.version_info[0]))' > py.test --cov=abp --cov-report=term-missing tests Done. https://codereview.adblockplus.org/29968569/diff/29969565/LICENSE File LICENSE (right): https://codereview.adblockplus.org/29968569/diff/29969565/LICENSE#newcode1 LICENSE:1: GNU LESSER GENERAL PUBLIC LICENSE On 2018/12/28 22:39:02, Sebastian Noack wrote: > Wrong license. This is LGPL, not the GPL. Oops. Done.
https://codereview.adblockplus.org/29968569/diff/29969585/README.md File README.md (right): https://codereview.adblockplus.org/29968569/diff/29969585/README.md#newcode33 README.md:33: <a id="rendering"></a> Injecting those HTML snippets across the Markdown-formatted README isn't great. It impairs the readability when reading the Markdown source code, makes it more difficult to review changes to the README, and requires authors to understand HTML (in addition to Markdown). Markdown's ability to use inline HTML should only be used as last resort if absolutely necessary. FWIW, GitHub automatically adds "id" attributes to headings when rendering Markdown. However, PyPI's Markdown renderer doesn't (even though it supposedly implements the same Markdown flavor). So unfortunately it seems if we want to have a table of contents in Markdown that is compatible with PyPI, injecting these HTML elements would be the only way. Another issue with this table of content is that it needs to be updated manually. FWIW, neither would be an issue if we'd use restructuredText instead of Markdown (like virtually every semi-popular Python project does). Among other neat features, it supports auto-generated table of contents (no inline HTML required), rendering the same way on GitHub and PyPI. I'm not sure if it's worth to convert the README to restructuredText at the current point, but then again adding a table of contents isn't probably either.
https://codereview.adblockplus.org/29968569/diff/29969585/abp/filters/sources.py File abp/filters/sources.py (right): https://codereview.adblockplus.org/29968569/diff/29969585/abp/filters/sources... abp/filters/sources.py:25: # The module was renamed in Python 3 Nit: Would you agree that this comment is somewhat redundant with the new #pragma comment above? https://codereview.adblockplus.org/29968569/diff/29969585/tests/test_fs_sourc... File tests/test_fs_source.py (right): https://codereview.adblockplus.org/29968569/diff/29969585/tests/test_fs_sourc... tests/test_fs_source.py:62: def test_websource_get_err(websource): Shouldn't this rather go in test_web_source.py?
https://codereview.adblockplus.org/29968569/diff/29969585/README.md File README.md (right): https://codereview.adblockplus.org/29968569/diff/29969585/README.md#newcode33 README.md:33: <a id="rendering"></a> On 2018/12/29 03:01:24, Sebastian Noack wrote: > Injecting those HTML snippets across the Markdown-formatted README isn't great. > It impairs the readability when reading the Markdown source code, makes it more > difficult to review changes to the README, and requires authors to understand > HTML (in addition to Markdown). Markdown's ability to use inline HTML should > only be used as last resort if absolutely necessary. > > FWIW, GitHub automatically adds "id" attributes to headings when rendering > Markdown. However, PyPI's Markdown renderer doesn't (even though it supposedly > implements the same Markdown flavor). So unfortunately it seems if we want to > have a table of contents in Markdown that is compatible with PyPI, injecting > these HTML elements would be the only way. > > Another issue with this table of content is that it needs to be updated > manually. FWIW, neither would be an issue if we'd use restructuredText instead > of Markdown (like virtually every semi-popular Python project does). Among other > neat features, it supports auto-generated table of contents (no inline HTML > required), rendering the same way on GitHub and PyPI. > > I'm not sure if it's worth to convert the README to restructuredText at the > current point, but then again adding a table of contents isn't probably either. I had a go translating the README to restructuredText. Here is how it looks like: https://gist.githubusercontent.com/snoack/09f798b3ab352b2cfa47b0e8b3624022/ra... Rendered: https://gist.github.com/snoack/09f798b3ab352b2cfa47b0e8b3624022 Compared to Markdown: https://gist.github.com/snoack/6ba2ab7dec6d40bde47f423988cb271d What do you think?
Hi Rosie and Sebastian! Thank you for working hard while I was having fun at the CCC. I like where this is going in general and Sebastian's proposal with RST looks good (also it seems that RST is more of a de facto standard for Python package README's). I don't have much to add to the review so far, just one idea about configuring python version for the coverage (see below). Also (not sure if it should be in this review, but seems relevant), what do you think about ditching DevEnvCommand from setup.py? I don't think anybody uses it and it doesn't seem to be compatible with how one calls virtualenv in Python 3 anyway. Cheers, Vasily https://codereview.adblockplus.org/29968569/diff/29969585/abp/filters/sources.py File abp/filters/sources.py (right): https://codereview.adblockplus.org/29968569/diff/29969585/abp/filters/sources... abp/filters/sources.py:25: # The module was renamed in Python 3 On 2018/12/29 03:24:56, Sebastian Noack wrote: > Nit: Would you agree that this comment is somewhat redundant with the new > #pragma comment above? I would agree, the pragma comment seems informative enough. https://codereview.adblockplus.org/29968569/diff/29969585/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29968569/diff/29969585/tox.ini#newcode12 tox.ini:12: python -c 'import sys; open(".coveragerc", "w").write("[report]\nexclude_lines = pragma: no py\{\} cover".format(sys.version_info[0]))' This is clever but I wonder if it would look less cryptic if we used `echo -e` or `printf` and environment variables instead (see https://tox.readthedocs.io/en/latest/config.html#injected-environment-variables). Something like: echo -e "[report]\nexclude_lines = pragma: no $TOX_ENV_NAME cover" >.coveragerc What do y'all think?
On 2019/01/02 18:32:28, Vasily Kuznetsov wrote: > what do you think about ditching DevEnvCommand from setup.py? Sounds good to me. https://codereview.adblockplus.org/29968569/diff/29969585/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29968569/diff/29969585/tox.ini#newcode12 tox.ini:12: python -c 'import sys; open(".coveragerc", "w").write("[report]\nexclude_lines = pragma: no py\{\} cover".format(sys.version_info[0]))' On 2019/01/02 18:32:28, Vasily Kuznetsov wrote: > This is clever but I wonder if it would look less cryptic if we used `echo -e` > or `printf` and environment variables instead (see > https://tox.readthedocs.io/en/latest/config.html#injected-environment-variables). > Something like: > > echo -e "[report]\nexclude_lines = pragma: no $TOX_ENV_NAME cover" > >.coveragerc > > What do y'all think? There are following problems with this approach: 1. TOX_ENV_NAME is fairly new, it was introduced with tox 3.4. For reference, Debian Buster (currently testing / next stable) as well as Ubuntu 18.10 still come with tox 2.5. So we would have to instruct Linux users to install the latest tox version from PyPI system-wide (something I genrally don't do), or to set up a virtualenv to run tox from. IMO, this isn't worth it if it's just for this feature. 2. We don't want to mark lines to be excluded on a specific interpreter but on either Python 2 or 3. 3. We want to avoid using UNIX commands for compatibility with Windows.
On 2019/01/02 20:42:31, Sebastian Noack wrote: > > There are following problems with this approach: > > 1. TOX_ENV_NAME is fairly new, it was introduced with tox 3.4. For reference, > Debian Buster (currently testing / next stable) as well as Ubuntu 18.10 still > come with tox 2.5. So we would have to instruct Linux users to install the > latest tox version from PyPI system-wide (something I genrally don't do), or to > set up a virtualenv to run tox from. IMO, this isn't worth it if it's just for > this feature. > 2. We don't want to mark lines to be excluded on a specific interpreter but on > either Python 2 or 3. > 3. We want to avoid using UNIX commands for compatibility with Windows. Allright, this sounds pretty convincing. Let's keep the python-based approach. Cheers, Vasily
Hi Vasily & Sebastian! Happy New Year, and I hope all is well. :) - I do like the README in ReStructuredText. - I removed the DevEnvCommand from setup.py, and the section of the README that discusses it. Should we mention anything else about the testing environment in the README? - I stuck with the Python-based approach for writing the .coveragerc file. And a few more notes below. Let me know if you see any other issues? https://codereview.adblockplus.org/29968569/diff/29969585/README.md File README.md (right): https://codereview.adblockplus.org/29968569/diff/29969585/README.md#newcode33 README.md:33: <a id="rendering"></a> On 2018/12/29 18:29:16, Sebastian Noack wrote: > On 2018/12/29 03:01:24, Sebastian Noack wrote: > > Injecting those HTML snippets across the Markdown-formatted README isn't > great. > > It impairs the readability when reading the Markdown source code, makes it > more > > difficult to review changes to the README, and requires authors to understand > > HTML (in addition to Markdown). Markdown's ability to use inline HTML should > > only be used as last resort if absolutely necessary. > > > > FWIW, GitHub automatically adds "id" attributes to headings when rendering > > Markdown. However, PyPI's Markdown renderer doesn't (even though it supposedly > > implements the same Markdown flavor). So unfortunately it seems if we want to > > have a table of contents in Markdown that is compatible with PyPI, injecting > > these HTML elements would be the only way. > > > > Another issue with this table of content is that it needs to be updated > > manually. FWIW, neither would be an issue if we'd use restructuredText instead > > of Markdown (like virtually every semi-popular Python project does). Among > other > > neat features, it supports auto-generated table of contents (no inline HTML > > required), rendering the same way on GitHub and PyPI. > > > > I'm not sure if it's worth to convert the README to restructuredText at the > > current point, but then again adding a table of contents isn't probably > either. > > I had a go translating the README to restructuredText. > > Here is how it looks like: > https://gist.githubusercontent.com/snoack/09f798b3ab352b2cfa47b0e8b3624022/ra... > > Rendered: > https://gist.github.com/snoack/09f798b3ab352b2cfa47b0e8b3624022 > > Compared to Markdown: > https://gist.github.com/snoack/6ba2ab7dec6d40bde47f423988cb271d > > What do you think? Yeah, looks good, thanks. I added a newline here and there for readability and consistency, so there should now be 2 newlines before each section. https://codereview.adblockplus.org/29968569/diff/29969585/abp/filters/sources.py File abp/filters/sources.py (right): https://codereview.adblockplus.org/29968569/diff/29969585/abp/filters/sources... abp/filters/sources.py:25: # The module was renamed in Python 3 On 2019/01/02 18:32:28, Vasily Kuznetsov wrote: > On 2018/12/29 03:24:56, Sebastian Noack wrote: > > Nit: Would you agree that this comment is somewhat redundant with the new > > #pragma comment above? > > I would agree, the pragma comment seems informative enough. Done. https://codereview.adblockplus.org/29968569/diff/29969585/tests/test_fs_sourc... File tests/test_fs_source.py (right): https://codereview.adblockplus.org/29968569/diff/29969585/tests/test_fs_sourc... tests/test_fs_source.py:62: def test_websource_get_err(websource): On 2018/12/29 03:24:56, Sebastian Noack wrote: > Shouldn't this rather go in test_web_source.py? Yeah, looks like it should, but this test is no longer necessary. (Since Patch Set 2, the WebSource.get() code it was testing was removed.) https://codereview.adblockplus.org/29968569/diff/29969585/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29968569/diff/29969585/tox.ini#newcode12 tox.ini:12: python -c 'import sys; open(".coveragerc", "w").write("[report]\nexclude_lines = pragma: no py\{\} cover".format(sys.version_info[0]))' On 2019/01/02 20:42:31, Sebastian Noack wrote: > On 2019/01/02 18:32:28, Vasily Kuznetsov wrote: > > This is clever but I wonder if it would look less cryptic if we used `echo -e` > > or `printf` and environment variables instead (see > > > https://tox.readthedocs.io/en/latest/config.html#injected-environment-variables). > > Something like: > > > > echo -e "[report]\nexclude_lines = pragma: no $TOX_ENV_NAME cover" > > >.coveragerc > > > > What do y'all think? > > There are following problems with this approach: > > 1. TOX_ENV_NAME is fairly new, it was introduced with tox 3.4. For reference, > Debian Buster (currently testing / next stable) as well as Ubuntu 18.10 still > come with tox 2.5. So we would have to instruct Linux users to install the > latest tox version from PyPI system-wide (something I genrally don't do), or to > set up a virtualenv to run tox from. IMO, this isn't worth it if it's just for > this feature. > 2. We don't want to mark lines to be excluded on a specific interpreter but on > either Python 2 or 3. > 3. We want to avoid using UNIX commands for compatibility with Windows. I briefly tried using tox environment variables, but ran into the issue that they're new with tox 3.4. Also, since we added 'Operating System :: OS Independent' to setup.py, we should try to keep it as generic as possible. But, I can add a comment so it's a bit less cryptic.
https://codereview.adblockplus.org/29968569/diff/29969585/README.md File README.md (right): https://codereview.adblockplus.org/29968569/diff/29969585/README.md#newcode193 README.md:193: (devenv) $ tox Running tox is still relevant, but you would run it form outside any virtualenv. https://codereview.adblockplus.org/29968569/diff/29971555/setup.py File setup.py (right): https://codereview.adblockplus.org/29968569/diff/29971555/setup.py#newcode31 setup.py:31: long_description_content_type='text/markdown', This must be changed to text/x-rst now. Respectively, README.md should also be renamed to README.rst. https://codereview.adblockplus.org/29968569/diff/29971555/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29968569/diff/29971555/tox.ini#newcode13 tox.ini:13: # containing the current Python version to a .coveragerc file Nit: The additional trailing spaces in the comment here seem inconsistent. (FWIW, IMO this comment is redundant anyway.)
https://codereview.adblockplus.org/29968569/diff/29969585/README.md File README.md (right): https://codereview.adblockplus.org/29968569/diff/29969585/README.md#newcode193 README.md:193: (devenv) $ tox On 2019/01/03 05:22:03, Sebastian Noack wrote: > Running tox is still relevant, but you would run it form outside any virtualenv. Done. https://codereview.adblockplus.org/29968569/diff/29971555/setup.py File setup.py (right): https://codereview.adblockplus.org/29968569/diff/29971555/setup.py#newcode31 setup.py:31: long_description_content_type='text/markdown', On 2019/01/03 05:22:03, Sebastian Noack wrote: > This must be changed to text/x-rst now. > > Respectively, README.md should also be renamed to README.rst. Done. https://codereview.adblockplus.org/29968569/diff/29971555/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29968569/diff/29971555/tox.ini#newcode13 tox.ini:13: # containing the current Python version to a .coveragerc file On 2019/01/03 05:22:03, Sebastian Noack wrote: > Nit: The additional trailing spaces in the comment here seem inconsistent. > (FWIW, IMO this comment is redundant anyway.) I'm guessing you mean the leading spaces here? Yeah, I can remove them for consistency. This comment may help save someone's time in the future trying to figure out why the line is here, but I don't feel too strongly about it either way.
LGTM https://codereview.adblockplus.org/29968569/diff/29971555/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29968569/diff/29971555/tox.ini#newcode13 tox.ini:13: # containing the current Python version to a .coveragerc file On 2019/01/03 21:32:29, rhowell wrote: > On 2019/01/03 05:22:03, Sebastian Noack wrote: > > Nit: The additional trailing spaces in the comment here seem inconsistent. > > (FWIW, IMO this comment is redundant anyway.) > > I'm guessing you mean the leading spaces here? Yeah, I can remove them for > consistency. Sorry, I meant leading spaces of course. > This comment may help save someone's time in the future trying to > figure out why the line is here, but I don't feel too strongly about it either > way. This comment just reiterates the code in the line below. In fact, it takes me more time to comprehend this comment than the code it describes. IMO, this is equivalent to adding a comment like "add one to the variable i" above "i += 1". But I don't care enough to keep arguing about this.
Hi Rosie and Sebastian,
Everything looks good now and I'm ready to approve the patch but I had problems
running the tests under pypy. When test_render_script.py runs flrender to test
it, Coverage gives out a warning that looks like this:
Coverage.py warning: Trace function changed, measurement is likely wrong:
None (trace-changed)
The warning gets mixed into the output of flrender and the test fails because
the output is not what the test expects. Have you seen anything like this?
I found an easy fix for it: Coverage warnings can be disabled based on type (see
https://coverage.readthedocs.io/en/coverage-4.4.2/config.html#run), so if we add
'\n[run]\ndisable_warnings=trace-changed' to what we write to .coveragerc, the
warning goes away and the test passes. However, if this is only something that
happens to me (maybe MacOS artifact, or caused by my version of pypy), I'd be
more reluctant to add this.
Cheers,
Vasily
On 2019/01/04 18:24:10, Vasily Kuznetsov wrote: > Hi Rosie and Sebastian, > > Everything looks good now and I'm ready to approve the patch but I had problems > running the tests under pypy. When test_render_script.py runs flrender to test > it, Coverage gives out a warning that looks like this: > > Coverage.py warning: Trace function changed, measurement is likely wrong: > None (trace-changed) > > The warning gets mixed into the output of flrender and the test fails because > the output is not what the test expects. Have you seen anything like this? > > I found an easy fix for it: Coverage warnings can be disabled based on type (see > https://coverage.readthedocs.io/en/coverage-4.4.2/config.html#run), so if we add > '\n[run]\ndisable_warnings=trace-changed' to what we write to .coveragerc, the > warning goes away and the test passes. However, if this is only something that > happens to me (maybe MacOS artifact, or caused by my version of pypy), I'd be > more reluctant to add this. > > Cheers, > Vasily Hi Vasily, and thanks for the feedback! That is odd, everything passes just fine from my end. I wonder if it's got something to do with using `pytest --cov` instead of just running `coverage`? According to the docs, pytest-cov should also be able to cover subprocesses, where coverage cannot. <https://pypi.org/project/pytest-cov/> But odd that it's only happening on PyPy. It may be something different, but that would be my first guess. I don't think Sebastian ran into the issue either. I asked Jon to test the patch, and he said it worked fine for him too (on Ubuntu 18.04). I'm running Mint 18.2 (based on Ubuntu 16.04).
On 2019/01/08 03:03:59, rhowell wrote: > I don't think Sebastian ran into the issue either. Well, previously I just ignored the InterpreterNotFound error. ;P I just installed PyPy 6.0.0 (2.7.13) from the Debian Buster repository, and when I run "tox -e pypy" I don't get that warning either.
On 2019/01/08 04:42:32, Sebastian Noack wrote: > On 2019/01/08 03:03:59, rhowell wrote: > > I don't think Sebastian ran into the issue either. > > Well, previously I just ignored the InterpreterNotFound error. ;P > I just installed PyPy 6.0.0 (2.7.13) from the Debian Buster repository, and when > I run "tox -e pypy" I don't get that warning either. I have rerun the tests with new PyPy (also 6.0.0) and saw no problems. I think we can ignore whatever was happening with the old one (it was 5.3.1, so quite outdated). LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
