Issue 29338156: Issue 3754 - Initial setup of the python-abp repo. (Closed)
|
Created:
March 11, 2016, 5:13 p.m. by Vasily Kuznetsov Modified:
March 17, 2016, 3:50 p.m. Reviewers:
Sebastian Noack CC:
mathias, Felix Dahlke Visibility:
Public. |
DescriptionIssue 3754 - Initial setup of the python-abp repo.
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address review comments (LICENSE.txt -> COPYING, .gitignore, encoding, license headers in empty fil… #
Total comments: 19
Patch Set 3 : Address review comments (ignore files, extra commands in setup.py, license text in readme, etc.) #Patch Set 4 : Move style and coverage checking to tox.ini, strip setup.py to only have 'devenv' command, remove n… #
Total comments: 8
Patch Set 5 : Update ignore files, describe testing in README.md and fix a typo in tox.ini. #
Total comments: 7
Patch Set 6 : Remove pytest from qa dependencies in tox.ini, change "filterlist" to "filter list" #
MessagesTotal messages: 23
Initial commit into python-abp. No actual code there yet, but please look at the lincensing stuff, structure, module naming, etc. Thanks
https://codereview.adblockplus.org/29338156/diff/29338157/.gitignore File .gitignore (right): https://codereview.adblockplus.org/29338156/diff/29338157/.gitignore#newcode2 .gitignore:2: .cache I think we should only ignore those folders at the top-level: /.cache/ /.coverage/ /.tox/ https://codereview.adblockplus.org/29338156/diff/29338157/.gitignore#newcode5 .gitignore:5: __ What is this entry about? https://codereview.adblockplus.org/29338156/diff/29338157/.gitignore#newcode6 .gitignore:6: python_abp.egg-info I'd rather avoid duplicating the project name. Also it seems to be sufficient as well to only consider the top-level here: /*.egg-info https://codereview.adblockplus.org/29338156/diff/29338157/LICENSE.txt File LICENSE.txt (right): https://codereview.adblockplus.org/29338156/diff/29338157/LICENSE.txt#newcode1 LICENSE.txt:1: GNU GENERAL PUBLIC LICENSE In all of our other repositories we include a file with the license under the name "COPYING", not "LICENSE.txt". You might want to change it for consistence. https://codereview.adblockplus.org/29338156/diff/29338157/abp/__init__.py File abp/__init__.py (right): https://codereview.adblockplus.org/29338156/diff/29338157/abp/__init__.py#new... abp/__init__.py:1: # -*- coding: utf-8 -*- (For compliance with PEP-8) we don't use a coding declaration but stick to ascii in new code. https://codereview.adblockplus.org/29338156/diff/29338157/abp/filters/__init_... File abp/filters/__init__.py (right): https://codereview.adblockplus.org/29338156/diff/29338157/abp/filters/__init_... abp/filters/__init__.py:3: # This file is part of Adblock Plus <https://adblockplus.org/>, No need for a license disclaimer in files that would be completely empty otherwise. https://codereview.adblockplus.org/29338156/diff/29338157/makefile File makefile (right): https://codereview.adblockplus.org/29338156/diff/29338157/makefile#newcode1 makefile:1: # Automation of common development operations. A makefile, doesn't seem appropriate for a Python project. Note that you can add additional commands to setup.py if you need to.
Thanks for the comments. https://codereview.adblockplus.org/29338156/diff/29338157/.gitignore File .gitignore (right): https://codereview.adblockplus.org/29338156/diff/29338157/.gitignore#newcode2 .gitignore:2: .cache On 2016/03/14 11:16:00, Sebastian Noack wrote: > I think we should only ignore those folders at the top-level: > > /.cache/ > /.coverage/ > /.tox/ Acknowledged. https://codereview.adblockplus.org/29338156/diff/29338157/.gitignore#newcode5 .gitignore:5: __ On 2016/03/14 11:16:01, Sebastian Noack wrote: > What is this entry about? This is a virtualenv that the makefile creates. It's used for testing and other things during development. https://codereview.adblockplus.org/29338156/diff/29338157/.gitignore#newcode6 .gitignore:6: python_abp.egg-info On 2016/03/14 11:16:00, Sebastian Noack wrote: > I'd rather avoid duplicating the project name. Also it seems to be sufficient as > well to only consider the top-level here: > > /*.egg-info Acknowledged. https://codereview.adblockplus.org/29338156/diff/29338157/LICENSE.txt File LICENSE.txt (right): https://codereview.adblockplus.org/29338156/diff/29338157/LICENSE.txt#newcode1 LICENSE.txt:1: GNU GENERAL PUBLIC LICENSE On 2016/03/14 11:16:01, Sebastian Noack wrote: > In all of our other repositories we include a file with the license under the > name "COPYING", not "LICENSE.txt". You might want to change it for consistence. Sure, will do. COPYING is also what Gnu people recommend. https://codereview.adblockplus.org/29338156/diff/29338157/abp/__init__.py File abp/__init__.py (right): https://codereview.adblockplus.org/29338156/diff/29338157/abp/__init__.py#new... abp/__init__.py:1: # -*- coding: utf-8 -*- On 2016/03/14 11:16:01, Sebastian Noack wrote: > (For compliance with PEP-8) we don't use a coding declaration but stick to ascii > in new code. That would be my preference too, but I missed it in the PEP-8 somehow. Will remove the coding declarations. https://codereview.adblockplus.org/29338156/diff/29338157/abp/filters/__init_... File abp/filters/__init__.py (right): https://codereview.adblockplus.org/29338156/diff/29338157/abp/filters/__init_... abp/filters/__init__.py:3: # This file is part of Adblock Plus <https://adblockplus.org/>, On 2016/03/14 11:16:01, Sebastian Noack wrote: > No need for a license disclaimer in files that would be completely empty > otherwise. Acknowledged. https://codereview.adblockplus.org/29338156/diff/29338157/makefile File makefile (right): https://codereview.adblockplus.org/29338156/diff/29338157/makefile#newcode1 makefile:1: # Automation of common development operations. On 2016/03/14 11:16:01, Sebastian Noack wrote: > A makefile, doesn't seem appropriate for a Python project. Note that you can add > additional commands to setup.py if you need to. The makefile is not necessary to build or install the module. It just saves typing by automating common tasks. So it's not an extra dependency that would annoy people that don't have make installed, just a helpful tool for those who do. Doing the same with setup.py is possible, but the amount of code would be greater, and I would probably need to create an additional python file to put this stuff in, which would clutter the project. Finally "make clean" is still shorter than "python setup.py clean". Finally, some high profile python projects use the approach with makefile, which makes me think that it's reasonably kosher. For example: * https://github.com/mitsuhiko/flask/blob/master/Makefile, * https://github.com/kennethreitz/requests/blob/master/Makefile So my preference would be to keep the makefile.
https://codereview.adblockplus.org/29338156/diff/29338157/.gitignore File .gitignore (right): https://codereview.adblockplus.org/29338156/diff/29338157/.gitignore#newcode5 .gitignore:5: __ On 2016/03/14 12:04:28, Vasily Kuznetsov wrote: > On 2016/03/14 11:16:01, Sebastian Noack wrote: > > What is this entry about? > > This is a virtualenv that the makefile creates. It's used for testing and other > things during development. Is it common practice to call it __, or should we rather give a more descriptive name to it? https://codereview.adblockplus.org/29338156/diff/29338157/makefile File makefile (right): https://codereview.adblockplus.org/29338156/diff/29338157/makefile#newcode1 makefile:1: # Automation of common development operations. Flask and Requests aren't the best examples for best practices if you ask me. Also except for the devenv command, the commands in here doesn't save much typing anyway. And that command could simply be implemented in setup.py as well. Using two seperate build systems, that is setup.py and Makefile seems redundant to me.
https://codereview.adblockplus.org/29338156/diff/29338157/.gitignore File .gitignore (right): https://codereview.adblockplus.org/29338156/diff/29338157/.gitignore#newcode5 .gitignore:5: __ On 2016/03/14 12:19:22, Sebastian Noack wrote: > On 2016/03/14 12:04:28, Vasily Kuznetsov wrote: > > On 2016/03/14 11:16:01, Sebastian Noack wrote: > > > What is this entry about? > > > > This is a virtualenv that the makefile creates. It's used for testing and > other > > things during development. > > Is it common practice to call it __, or should we rather give a more descriptive > name to it? The only special thing about naming it __ is that when the virtualenv is activated, the shell prompt prefix becomes [python-abp] (by the name of the parent directory, that is working copy root). However, it seems that the virtualenv is not really used like this very much, so perhaps a more descriptive name would be better. How about I just call it "devenv"? https://codereview.adblockplus.org/29338156/diff/29338157/makefile File makefile (right): https://codereview.adblockplus.org/29338156/diff/29338157/makefile#newcode1 makefile:1: # Automation of common development operations. On 2016/03/14 12:19:22, Sebastian Noack wrote: > Flask and Requests aren't the best examples for best practices if you ask me. > Also except for the devenv command, the commands in here doesn't save much > typing anyway. And that command could simply be implemented in setup.py as well. > Using two seperate build systems, that is setup.py and Makefile seems redundant > to me. Ok, let me try to implement the same functionality in setup.py and we see how it looks.
I uploaded an updated version: 1. LICENSE.txt is now called copying, 2. Encoding headers are gone from python files as well as license header in that otherwise empty __init__.py, 3. .gitignore is fixed, 4. Virtualenv is now called "devenv" instead of "__". 5. Makefile is removed and converted to python-based automation in setup.py. I still think it was more terse and elegant in makefile format, but this also work and I agree that one build automation system is better than two.
https://codereview.adblockplus.org/29338156/diff/29338299/.gitignore File .gitignore (right): https://codereview.adblockplus.org/29338156/diff/29338299/.gitignore#newcode1 .gitignore:1: **/*.pyc Why not simply *.pyc? Moreover, in our other repos we also account for *.pyo. https://codereview.adblockplus.org/29338156/diff/29338299/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29338156/diff/29338299/.hgignore#newcode3 .hgignore:3: .cache These directories are only matched at the top-level in .gitignore. To get the same behavior with hg you can use regular expression syntax, however you have to move the regular expression patterns above "syntax: glob". https://codereview.adblockplus.org/29338156/diff/29338299/README.md File README.md (right): https://codereview.adblockplus.org/29338156/diff/29338299/README.md#newcode5 README.md:5: ## Copyright and license For our other projects we don't have a license disclaimer in the README. https://codereview.adblockplus.org/29338156/diff/29338299/abp/__init__.py File abp/__init__.py (right): https://codereview.adblockplus.org/29338156/diff/29338299/abp/__init__.py#new... abp/__init__.py:16: __import__('pkg_resources').declare_namespace(__name__) I rarely used that stuff. So what is the benefit/purpose of declaring a namespace with pkg_resources? Also any reason you hide the import while it's not even optional? https://codereview.adblockplus.org/29338156/diff/29338299/setup.py File setup.py (right): https://codereview.adblockplus.org/29338156/diff/29338299/setup.py#newcode16 setup.py:16: """A library for working with Adblock Plus filterlists.""" This doc string seems to be misleading, as this file isn't the library mentioned but it's setup script. https://codereview.adblockplus.org/29338156/diff/29338299/setup.py#newcode18 setup.py:18: from __future__ import print_function It seems you don't use any print statements in here. https://codereview.adblockplus.org/29338156/diff/29338299/setup.py#newcode130 setup.py:130: 'devenv': make_command(devenv), As I said before, most of the commands you added in here seem superfluous to me, e.g. running "python setup.py testall" cetrtainly isn't any more convenient than simply running "tox". And in particular the "devclean" command requires quite some complexity and hard-coded information, while somebody could simply run |hg clean --all| or |git clean -x| instead. While the logic for most other commands isn't even specific to this project. So do you copy that boilerplate into every single Python project? That kind of cargo cult programming is something we generally don't do. IMO, the "devenv" command would be the only one that might be justified. https://codereview.adblockplus.org/29338156/diff/29338299/setup.py#newcode151 setup.py:151: 'Programming Language :: Python :: 3.3', If you list Python 3.3 and 3.4 you might want to also list them in tox.ini
https://codereview.adblockplus.org/29338156/diff/29338299/.gitignore File .gitignore (right): https://codereview.adblockplus.org/29338156/diff/29338299/.gitignore#newcode1 .gitignore:1: **/*.pyc On 2016/03/15 12:18:06, Sebastian Noack wrote: > Why not simply *.pyc? > > Moreover, in our other repos we also account for *.pyo. Acknowledged. https://codereview.adblockplus.org/29338156/diff/29338299/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29338156/diff/29338299/.hgignore#newcode3 .hgignore:3: .cache On 2016/03/15 12:18:06, Sebastian Noack wrote: > These directories are only matched at the top-level in .gitignore. To get the > same behavior with hg you can use regular expression syntax, however you have to > move the regular expression patterns above "syntax: glob". Acknowledged. https://codereview.adblockplus.org/29338156/diff/29338299/README.md File README.md (right): https://codereview.adblockplus.org/29338156/diff/29338299/README.md#newcode5 README.md:5: ## Copyright and license On 2016/03/15 12:18:07, Sebastian Noack wrote: > For our other projects we don't have a license disclaimer in the README. Makes sense. With COPYING file at the top level it's reasonably redundant. https://codereview.adblockplus.org/29338156/diff/29338299/abp/__init__.py File abp/__init__.py (right): https://codereview.adblockplus.org/29338156/diff/29338299/abp/__init__.py#new... abp/__init__.py:16: __import__('pkg_resources').declare_namespace(__name__) On 2016/03/15 12:18:07, Sebastian Noack wrote: > I rarely used that stuff. So what is the benefit/purpose of declaring a > namespace with pkg_resources? > > Also any reason you hide the import while it's not even optional? This is needed in case we would have two separate python packages that contain modules in abp namespace and they would be installed with easy_install. For example if we have "python-abp" that contains abp.filters and "python-abp-foo" that contains abp.foo. Easy_install would put them into two separate eggs and then if we "import abp.foo" it would only search the first one and perhaps not find "abp.foo". Here's an example of this made just to demonstrate how it works: https://github.com/kvas-it/test-namespace-pkg Clearly, at the moment we don't need to worry about all of this, but not declaring the namespace carries the danger that if in the future we introduce python-abp-foo (maybe not with this name) then if people just "easy_install python-abp-foo" on top of existing setup with python-abp, they will find that "import abp.foo" mysteriously does not work and they need to update python-abp to fix it (assuming that we have released a new version which declares the namespace by that time). This is ugly. Now one can argue that this is all very hypothetical and perhaps people should not be using easy_install in the first place (pip handles the situation correctly without the namespace declaration) but declaring the namespace is not a big deal and I've never seen a python installation where the slowdown from it would be significant. So my usual choice is: just declare the namespace, forget about it and sleep well at night. As for using __import__ instead of "import pkg_resources" -- it makes the whole thing a neat one-liner which is one fewer line of meaningless code, whose only purpose is to fix this glitch of python module system. So basically the reason is purely cosmetic. https://codereview.adblockplus.org/29338156/diff/29338299/setup.py File setup.py (right): https://codereview.adblockplus.org/29338156/diff/29338299/setup.py#newcode16 setup.py:16: """A library for working with Adblock Plus filterlists.""" On 2016/03/15 12:18:07, Sebastian Noack wrote: > This doc string seems to be misleading, as this file isn't the library mentioned > but it's setup script. It's here to be used in the long_description later, but yeah, misleading indeed. I'll move the content of the docstring there directly instead. https://codereview.adblockplus.org/29338156/diff/29338299/setup.py#newcode18 setup.py:18: from __future__ import print_function On 2016/03/15 12:18:07, Sebastian Noack wrote: > It seems you don't use any print statements in here. Acknowledged. https://codereview.adblockplus.org/29338156/diff/29338299/setup.py#newcode130 setup.py:130: 'devenv': make_command(devenv), On 2016/03/15 12:18:07, Sebastian Noack wrote: > As I said before, most of the commands you added in here seem superfluous to me, > e.g. running "python setup.py testall" cetrtainly isn't any more convenient than > simply running "tox". > > And in particular the "devclean" command requires quite some complexity and > hard-coded information, while somebody could simply run |hg clean --all| or |git > clean -x| instead. > > While the logic for most other commands isn't even specific to this project. So > do you copy that boilerplate into every single Python project? That kind of > cargo cult programming is something we generally don't do. > > IMO, the "devenv" command would be the only one that might be justified. Pretty much none of the commands are really specific to this project. The idea is that anybody who hacks on this code can run the build, tests, coverage reports etc. the same way and get hopefully the same results. I don't think there's any cargo cult in this. I would probably agree that things like diffpep8 and htmlcov are going a bit too far, but things like syntaxcheck and testcov are generally useful, and they in a way define a coding standard for the project. About devclean -- you're right. So how about I remove diffpep8, htmlcov and devclean and keep the rest? https://codereview.adblockplus.org/29338156/diff/29338299/setup.py#newcode151 setup.py:151: 'Programming Language :: Python :: 3.3', On 2016/03/15 12:18:07, Sebastian Noack wrote: > If you list Python 3.3 and 3.4 you might want to also list them in tox.ini Good point. I'd actually rather remove them from here. I don't think there's a good reason to use Python 3.3 or 3.4 at this moment. What do you think?
https://codereview.adblockplus.org/29338156/diff/29338299/abp/__init__.py File abp/__init__.py (right): https://codereview.adblockplus.org/29338156/diff/29338299/abp/__init__.py#new... abp/__init__.py:16: __import__('pkg_resources').declare_namespace(__name__) On 2016/03/15 13:14:35, Vasily Kuznetsov wrote: > On 2016/03/15 12:18:07, Sebastian Noack wrote: > > I rarely used that stuff. So what is the benefit/purpose of declaring a > > namespace with pkg_resources? > > > > Also any reason you hide the import while it's not even optional? > > This is needed in case we would have two separate python packages that contain > modules in abp namespace and they would be installed with easy_install. For > example if we have "python-abp" that contains abp.filters and "python-abp-foo" > that contains abp.foo. Easy_install would put them into two separate eggs and > then if we "import abp.foo" it would only search the first one and perhaps not > find "abp.foo". > > Here's an example of this made just to demonstrate how it works: > https://github.com/kvas-it/test-namespace-pkg > > Clearly, at the moment we don't need to worry about all of this, but not > declaring the namespace carries the danger that if in the future we introduce > python-abp-foo (maybe not with this name) then if people just "easy_install > python-abp-foo" on top of existing setup with python-abp, they will find that > "import abp.foo" mysteriously does not work and they need to update python-abp > to fix it (assuming that we have released a new version which declares the > namespace by that time). This is ugly. > > Now one can argue that this is all very hypothetical and perhaps people should > not be using easy_install in the first place (pip handles the situation > correctly without the namespace declaration) but declaring the namespace is not > a big deal and I've never seen a python installation where the slowdown from it > would be significant. So my usual choice is: just declare the namespace, forget > about it and sleep well at night. > > As for using __import__ instead of "import pkg_resources" -- it makes the whole > thing a neat one-liner which is one fewer line of meaningless code, whose only > purpose is to fix this glitch of python module system. So basically the reason > is purely cosmetic. FWIW, I would rather ignore that scenario, for reasons you already mentioned: 1. We don't need to bother until we will bundle partial packages. 2. If we do we can still add a namespace declaration here. 3. Nobody is (or should) use easy_install nowadays anyway and pip doesn't seem to require namespace declarations. That is not about performance, but about unnecessary code. So people that try to understand what the code in this repo does will stumble upon this piece, and might wonder similarly than I did what that is about. Anyway, it's not a too important detail, to argue about it. So if you insist keep it in there, but using __import__ magic just to avoid an additional line of code, doesn't sound like a good practice. Also it probably would make sense to make the dependency on pkg_resources optional. https://codereview.adblockplus.org/29338156/diff/29338299/setup.py File setup.py (right): https://codereview.adblockplus.org/29338156/diff/29338299/setup.py#newcode130 setup.py:130: 'devenv': make_command(devenv), On 2016/03/15 13:14:35, Vasily Kuznetsov wrote: > On 2016/03/15 12:18:07, Sebastian Noack wrote: > > As I said before, most of the commands you added in here seem superfluous to > me, > > e.g. running "python setup.py testall" cetrtainly isn't any more convenient > than > > simply running "tox". > > > > And in particular the "devclean" command requires quite some complexity and > > hard-coded information, while somebody could simply run |hg clean --all| or > |git > > clean -x| instead. > > > > While the logic for most other commands isn't even specific to this project. > So > > do you copy that boilerplate into every single Python project? That kind of > > cargo cult programming is something we generally don't do. > > > > IMO, the "devenv" command would be the only one that might be justified. > > Pretty much none of the commands are really specific to this project. The idea > is that anybody who hacks on this code can run the build, tests, coverage > reports etc. the same way and get hopefully the same results. I don't think > there's any cargo cult in this. > > I would probably agree that things like diffpep8 and htmlcov are going a bit too > far, but things like syntaxcheck and testcov are generally useful, and they in a > way define a coding standard for the project. > > About devclean -- you're right. > > So how about I remove diffpep8, htmlcov and devclean and keep the rest? IMO, the tox script should run py.test --cov anyway. And if you want to use flake8, it should probably be called from tox as well. So people working on that code would only have to run tox. That's not only even more convenient than those custom commands, but also makes sure that people run the whole suite on all supported Python versions. https://codereview.adblockplus.org/29338156/diff/29338299/setup.py#newcode151 setup.py:151: 'Programming Language :: Python :: 3.3', On 2016/03/15 13:14:35, Vasily Kuznetsov wrote: > On 2016/03/15 12:18:07, Sebastian Noack wrote: > > If you list Python 3.3 and 3.4 you might want to also list them in tox.ini > > Good point. I'd actually rather remove them from here. I don't think there's a > good reason to use Python 3.3 or 3.4 at this moment. What do you think? So far we didn't care about supporting outdated Python versions, since we have Python 2.7 anyway wherever we run our code, and testing additional versions would have been a hassle. However, in the case here, on the other hand, with py.test and tox, testing a set of Python versions wouldn't be a big deal. And since we have to support Python 2 and 3, we cannot use any new features added after Python 3.3 without a fallback, anyway. But then again, if there is nobody using that code on Python 3.3 or 3.4 it doesn't really matter. I leave it up to you.
In the absence of further comments I implemented the changes as I proposed. Let me know if you see anything else that can/should be improved. Thank you
I moved most of the automation to tox.ini so only 'devenv' command is left in setup.py. Also removed namespace declaration from abp/__init__.py -- after all consideration, it's thinking too much ahead at this point. Thanks for the comments
https://codereview.adblockplus.org/29338156/diff/29338379/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29338156/diff/29338379/.hgignore#newcode1 .hgignore:1: ^.cache$ To be 100% consistent with the .gitignore you'd have to add the trailing slashes. https://codereview.adblockplus.org/29338156/diff/29338379/.hgignore#newcode8 .hgignore:8: **/*.pyc Also for consistency with the .gitignore please remove **/. https://codereview.adblockplus.org/29338156/diff/29338379/README.md File README.md (right): https://codereview.adblockplus.org/29338156/diff/29338379/README.md#newcode3 README.md:3: A library for working with Adblock Plus filterlists. Perhaps you should mention tox here, maybe add a section about testing. https://codereview.adblockplus.org/29338156/diff/29338379/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29338156/diff/29338379/tox.ini#newcode11 tox.ini:11: # find .coverage file. Typo: "can find the .coverage file."
TLDR: Agree with comments, new patch coming. https://codereview.adblockplus.org/29338156/diff/29338379/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29338156/diff/29338379/.hgignore#newcode1 .hgignore:1: ^.cache$ On 2016/03/16 00:18:41, Sebastian Noack wrote: > To be 100% consistent with the .gitignore you'd have to add the trailing > slashes. Actually .coverage is a file, not a directory, so it should not have a slash in .gitignore. Will correct it there. For the others -- will replace trailing $ with a slash. https://codereview.adblockplus.org/29338156/diff/29338379/.hgignore#newcode8 .hgignore:8: **/*.pyc On 2016/03/16 00:18:41, Sebastian Noack wrote: > Also for consistency with the .gitignore please remove **/. Acknowledged. https://codereview.adblockplus.org/29338156/diff/29338379/README.md File README.md (right): https://codereview.adblockplus.org/29338156/diff/29338379/README.md#newcode3 README.md:3: A library for working with Adblock Plus filterlists. On 2016/03/16 00:18:41, Sebastian Noack wrote: > Perhaps you should mention tox here, maybe add a section about testing. I was thinking that later this kind of information would go into something like HACKING.md. But at this point, it's probably not worth creating a separate file for it and it could already be put here. Since I'm not giving full contributor guidelines at the moment, the section will be called "Testing". https://codereview.adblockplus.org/29338156/diff/29338379/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29338156/diff/29338379/tox.ini#newcode11 tox.ini:11: # find .coverage file. On 2016/03/16 00:18:41, Sebastian Noack wrote: > Typo: "can find the .coverage file." I'm not a native speaker but my intuition was that "the" is not needed here. The internet, however, seems to agree with you.
https://codereview.adblockplus.org/29338156/diff/29338452/setup.py File setup.py (right): https://codereview.adblockplus.org/29338156/diff/29338452/setup.py#newcode40 setup.py:40: subprocess.check_call([PIP, 'install'] + DEV_DEPENDENCIES) Wait, wouldn't that install these packages globally rather than in the devenv? https://codereview.adblockplus.org/29338156/diff/29338452/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29338156/diff/29338452/tox.ini#newcode17 tox.ini:17: pytest IIRC, dependencies are inherited from testenv here. So this line would be redundant, but better double check.
https://codereview.adblockplus.org/29338156/diff/29338452/setup.py File setup.py (right): https://codereview.adblockplus.org/29338156/diff/29338452/setup.py#newcode40 setup.py:40: subprocess.check_call([PIP, 'install'] + DEV_DEPENDENCIES) On 2016/03/16 16:03:59, Sebastian Noack wrote: > Wait, wouldn't that install these packages globally rather than in the devenv? It will install in the devenv because (line 22) "PIP = path.join(DEVENV, 'bin', 'pip')" and than the pip from the devenv installs into its devenv. https://codereview.adblockplus.org/29338156/diff/29338452/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29338156/diff/29338452/tox.ini#newcode17 tox.ini:17: pytest On 2016/03/16 16:03:59, Sebastian Noack wrote: > IIRC, dependencies are inherited from testenv here. So this line would be > redundant, but better double check. It works without this line although I'm not sure if it's because pytest-cov requires pytest or because of inheritance. When it installs the dependencies it now says "qa installdeps: pytest-cov, flake8" so perhaps the former. But anyway, this line seems to be redundant.
https://codereview.adblockplus.org/29338156/diff/29338452/setup.py File setup.py (right): https://codereview.adblockplus.org/29338156/diff/29338452/setup.py#newcode40 setup.py:40: subprocess.check_call([PIP, 'install'] + DEV_DEPENDENCIES) On 2016/03/16 16:29:35, Vasily Kuznetsov wrote: > On 2016/03/16 16:03:59, Sebastian Noack wrote: > > Wait, wouldn't that install these packages globally rather than in the devenv? > > It will install in the devenv because (line 22) "PIP = path.join(DEVENV, 'bin', > 'pip')" and than the pip from the devenv installs into its devenv. Are you sure that works even without virtualenv beeing activated?
https://codereview.adblockplus.org/29338156/diff/29338452/setup.py File setup.py (right): https://codereview.adblockplus.org/29338156/diff/29338452/setup.py#newcode40 setup.py:40: subprocess.check_call([PIP, 'install'] + DEV_DEPENDENCIES) On 2016/03/16 16:44:59, Sebastian Noack wrote: > On 2016/03/16 16:29:35, Vasily Kuznetsov wrote: > > On 2016/03/16 16:03:59, Sebastian Noack wrote: > > > Wait, wouldn't that install these packages globally rather than in the > devenv? > > > > It will install in the devenv because (line 22) "PIP = path.join(DEVENV, > 'bin', > > 'pip')" and than the pip from the devenv installs into its devenv. > > Are you sure that works even without virtualenv beeing activated? Yes, it definitely works. I ran this thing many times, it's not here just for the looks ;)
https://codereview.adblockplus.org/29338156/diff/29338452/README.md File README.md (right): https://codereview.adblockplus.org/29338156/diff/29338452/README.md#newcode3 README.md:3: A library for working with Adblock Plus filterlists. Nit: "filter lists" should be two words.
On 2016/03/17 09:25:33, Felix Dahlke wrote: > https://codereview.adblockplus.org/29338156/diff/29338452/README.md > File README.md (right): > > https://codereview.adblockplus.org/29338156/diff/29338452/README.md#newcode3 > README.md:3: A library for working with Adblock Plus filterlists. > Nit: "filter lists" should be two words. Ok. I just saw it written together in some places and I assumed that that's the preferred way to write it. Will change it everywhere.
LGTM
Changed "filterlist" to "filter list" in README and setup.py; also removed "pytest" from dependencies of qa environment in tox.ini.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
