Issue 29359949: Noissue - Add README [buildtools] (Closed)
DescriptionNoissue - Add README
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed feedback #Patch Set 3 : Don't bother document how to install Tox #MessagesTotal messages: 11
Patch Set 1 Since I didn't know how to run the tests for the repository I figured I should add a README.
Thanks for the README, Dave. Definitely less confusing this way. A couple of nits, see below. https://codereview.adblockplus.org/29359949/diff/29359950/README.md File README.md (right): https://codereview.adblockplus.org/29359949/diff/29359950/README.md#newcode13 README.md:13: instructions Nit: period at the end of the sentence. https://codereview.adblockplus.org/29359949/diff/29359950/README.md#newcode21 README.md:21: First install flake8, flake8-abp and tox: It should not be necessary to install flake8 globally in order to run the tests with Tox. Tox installs it automatically inside of the virtualenv that it creates for testing. Same for flake8-abp. So the only thing that is needed is tox itself.
Patch Set 2 : Addressed feedback https://codereview.adblockplus.org/29359949/diff/29359950/README.md File README.md (right): https://codereview.adblockplus.org/29359949/diff/29359950/README.md#newcode13 README.md:13: instructions On 2016/10/26 16:15:34, Vasily Kuznetsov wrote: > Nit: period at the end of the sentence. Done. https://codereview.adblockplus.org/29359949/diff/29359950/README.md#newcode21 README.md:21: First install flake8, flake8-abp and tox: On 2016/10/26 16:15:34, Vasily Kuznetsov wrote: > It should not be necessary to install flake8 globally in order to run the tests > with Tox. Tox installs it automatically inside of the virtualenv that it creates > for testing. Same for flake8-abp. So the only thing that is needed is tox > itself. Cool. Done.
I wonder whether it is worth to document how to install tox, in all of our Python-based repositories. The process of installing and running to is consistent through all of our repos and most of the (modern) Python ecosystem, that it is so standard that you could as well document how to install pip (or even Python), IMO.
On 2016/10/26 22:08:40, Sebastian Noack wrote: > I wonder whether it is worth to document how to install tox, in all of our > Python-based repositories. The process of installing and running to is > consistent through all of our repos and most of the (modern) Python ecosystem, > that it is so standard that you could as well document how to install pip (or > even Python), IMO. I take your point that the documentation here is kind of redundant for people that already know how to run these tests but it would have saved me the ~30mins that I wasted yesterday. I'd never used flake8 or tox before and had to search around for other documentation to figure it out. (See the first patch set where I thought flake8-adblockplus needed to be installed manually for example.)
On 2016/10/27 08:36:17, kzar wrote: > On 2016/10/26 22:08:40, Sebastian Noack wrote: > > I wonder whether it is worth to document how to install tox, in all of our > > Python-based repositories. The process of installing and running to is > > consistent through all of our repos and most of the (modern) Python ecosystem, > > that it is so standard that you could as well document how to install pip (or > > even Python), IMO. > > I take your point that the documentation here is kind of redundant for people > that already know how to run these tests but it would have saved me the ~30mins > that I wasted yesterday. I'd never used flake8 or tox before and had to search > around for other documentation to figure it out. (See the first patch set where > I thought flake8-adblockplus needed to be installed manually for example.) I agree with both of your points and I'd also like to note that the process for installing tox and pip can actually be different depending on your setup. That can also be done via apt-get or using a virtualenv or (on MacOS) via homebrew or ... well, you get the idea. I wish I could say "just post a link to https://tox.readthedocs.io/en/latest/install.html" but the description there is as good as what Dave wrote. All in all, what we have now seems like a good compromise.
On 2016/10/27 09:35:13, Vasily Kuznetsov wrote: > On 2016/10/27 08:36:17, kzar wrote: > > On 2016/10/26 22:08:40, Sebastian Noack wrote: > > > I wonder whether it is worth to document how to install tox, in all of our > > > Python-based repositories. The process of installing and running to is > > > consistent through all of our repos and most of the (modern) Python > ecosystem, > > > that it is so standard that you could as well document how to install pip > (or > > > even Python), IMO. > > > > I take your point that the documentation here is kind of redundant for people > > that already know how to run these tests but it would have saved me the > ~30mins > > that I wasted yesterday. I'd never used flake8 or tox before and had to search > > around for other documentation to figure it out. (See the first patch set > where > > I thought flake8-adblockplus needed to be installed manually for example.) > > I agree with both of your points and I'd also like to note that the process for > installing tox and pip can actually be different depending on your setup. That > can also be done via apt-get or using a virtualenv or (on MacOS) via homebrew or > ... well, you get the idea. I wish I could say "just post a link to > https://tox.readthedocs.io/en/latest/install.html%22 but the description there is > as good as what Dave wrote. All in all, what we have now seems like a good > compromise. Note that in other repositories we just refer to tox without any installation instructions: https://hg.adblockplus.org/python-abp/file/tip/README.md https://hg.adblockplus.org/sitescripts/file/tip/README.md So why isn't that sufficient here? I doubt anybody would waste a lot of time on figuring out how to install tox, once you know that this is the only thing you need. In fact, as Vasily indicated, the installation instructions would be useless for me for example, as I need essential tools like tox using apt-get and everything inside a virtualenv wich in this ccase is automated by tox. Also note that tox is even referenced to in our coding practices, so everybody who works on our Python code should be aware of it: https://adblockplus.org/coding-style#python
On 2016/10/27 09:58:59, Sebastian Noack wrote: > So why isn't that sufficient here? I doubt anybody would waste a lot of time on > figuring out how to install tox, once you know that this is the only thing you > need. In fact, as Vasily indicated, the installation instructions would be > useless for me for example, as I need essential tools like tox using apt-get and > everything inside a virtualenv wich in this ccase is automated by tox. Sorry, that sentence got a little messed up: ...as I install essential tools like tox using apt-get and everything else inside a virtualenv which in this case is automated by tox.
Patch Set 3 : Don't bother document how to install Tox
LGTM
LGTM 
|
