Noissue - Added flake8-docstrings (codingtools)

Noissue - Added flake8-docstrings (codingtools)

[Sebastian Noack, 2017-10-09 02:08:59]

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/flake8_...
File flake8-eyeo/flake8_eyeo.py (right):

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/flake8_...
flake8-eyeo/flake8_eyeo.py:402: pass  # Ignore multiline strings
These are checked by flake8-docstrings now.

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini
File flake8-eyeo/tox.ini (right):

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
flake8-eyeo/tox.ini:14: [testenv:flake8]
This is somewhat unrelated, but it seems to make most sense for the tox setups
where we have many environments, to lint our code separately, instead of each
time for each Python version supported.

https://codereview.adblockplus.org/29569634/diff/29569635/patchconv/tox.ini
File patchconv/tox.ini (right):

https://codereview.adblockplus.org/29569634/diff/29569635/patchconv/tox.ini#n...
patchconv/tox.ini:2: envlist = py27,py35,py36,flake8
py36 was missing here. I added it while unifying the tox configurations.

[Vasily Kuznetsov, 2017-10-09 21:16:15]

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini
File flake8-eyeo/tox.ini (right):

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
flake8-eyeo/tox.ini:14: [testenv:flake8]
On 2017/10/09 02:08:58, Sebastian Noack wrote:
> This is somewhat unrelated, but it seems to make most sense for the tox setups
> where we have many environments, to lint our code separately, instead of each
> time for each Python version supported.

Indeed, it doesn't make sense to lint the code with the old version of Flake8
since we don't care what it thinks if the new version likes the code.

However, as Flake8 is tied to the version of Python on which it runs [1], in
order to confirm that code is Python2-kosher and also Python3-kosher, we might
need to run Flake8 on both Python2 and Python3. It's possible that the only
difference between running it on different interpreters is the ability to parse
Python3-syntax (irrelevant here since we can't use it), but I haven't found any
source that would confirm or disconfirm this, so if we want to be on the safe
side, doing both seems advisable. Or maybe that's too paranoid... I'm not sure.

In any case it probably makes sense to select a specific version of Python in
the linting environment to ensure consistency.

[1] http://flake8.pycqa.org/en/latest/ (see the note at the top of the page)

[tlucas, 2017-10-09 23:39:39]

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini
File flake8-eyeo/tox.ini (right):

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
flake8-eyeo/tox.ini:2: envlist = py{27,35,36}-flake8{2,3},flake8
In case you decide to stick to your separation of testing and linting, we won't
need additional environments for flake82 / flake83 (since flake8 won't be run
anyway)

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
flake8-eyeo/tox.ini:8: deps =
Same as above, if you stick to your separation we won't even need to install
flake8 in the environments where only the tests are run.

[Sebastian Noack, 2017-10-10 05:34:30]

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini
File flake8-eyeo/tox.ini (right):

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
flake8-eyeo/tox.ini:2: envlist = py{27,35,36}-flake8{2,3},flake8
On 2017/10/09 23:39:38, tlucas wrote:
> In case you decide to stick to your separation of testing and linting, we
won't
> need additional environments for flake82 / flake83 (since flake8 won't be run
> anyway)

Well, this is the flake8-eyeo extension. The py{27,35,36}-flake8{2,3}
environments don't lint this code, but run the tests. And obviously we have to
test the extension with all supported versions of Python and flake8. We can drop
support for flake8 2 now, but this is out of the scope of this change.

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
flake8-eyeo/tox.ini:8: deps =
On 2017/10/09 23:39:38, tlucas wrote:
> Same as above, if you stick to your separation we won't even need to install
> flake8 in the environments where only the tests are run.

Again, this is the environment for the tests (not for linting), and since this
is a flake8 plugin, of course we need flake8 to test it.

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
flake8-eyeo/tox.ini:14: [testenv:flake8]
On 2017/10/09 21:16:14, Vasily Kuznetsov wrote:
> On 2017/10/09 02:08:58, Sebastian Noack wrote:
> > This is somewhat unrelated, but it seems to make most sense for the tox
setups
> > where we have many environments, to lint our code separately, instead of
each
> > time for each Python version supported.
> 
> Indeed, it doesn't make sense to lint the code with the old version of Flake8
> since we don't care what it thinks if the new version likes the code.
> 
> However, as Flake8 is tied to the version of Python on which it runs [1], in
> order to confirm that code is Python2-kosher and also Python3-kosher, we might
> need to run Flake8 on both Python2 and Python3. It's possible that the only
> difference between running it on different interpreters is the ability to
parse
> Python3-syntax (irrelevant here since we can't use it), but I haven't found
any
> source that would confirm or disconfirm this, so if we want to be on the safe
> side, doing both seems advisable. Or maybe that's too paranoid... I'm not
sure.
> 
> In any case it probably makes sense to select a specific version of Python in
> the linting environment to ensure consistency.
> 
> [1] http://flake8.pycqa.org/en/latest/ (see the note at the top of the page)

The note in the flake8 documentation merely reminds you that when you use flake8
on an older Python version, that it cannot parse (or at least properly
understand) language features that have been added in newer versions of Python.
This, however, doesn't matter if your code is backwards compatible with Python
2, as you are stuck with the feature subset of all supported Python versions.

Regardless, I think it is sufficient to lint code that is compatible with Python
3, on Python 3 only. If the code looks good in Python 3, it looks good for the
future. And as long as it still runs on Python 2 (if required), I don't care if
in rare cases it might cause some additional linter warnings there. At some
point, support for Python 2 will be dropped, anyway.

It seems, by default, tox is using the latest available Python interpreter, if
not otherwise specified. So IMO we are all good.

[tlucas, 2017-10-10 08:49:59]

On 2017/10/10 05:34:30, Sebastian Noack wrote:
> https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini
> File flake8-eyeo/tox.ini (right):
> 
>
https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
> flake8-eyeo/tox.ini:2: envlist = py{27,35,36}-flake8{2,3},flake8
> On 2017/10/09 23:39:38, tlucas wrote:
> > In case you decide to stick to your separation of testing and linting, we
> won't
> > need additional environments for flake82 / flake83 (since flake8 won't be
run
> > anyway)
> 
> Well, this is the flake8-eyeo extension. The py{27,35,36}-flake8{2,3}
> environments don't lint this code, but run the tests. And obviously we have to
> test the extension with all supported versions of Python and flake8. We can
drop
> support for flake8 2 now, but this is out of the scope of this change.
> 
>
https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
> flake8-eyeo/tox.ini:8: deps =
> On 2017/10/09 23:39:38, tlucas wrote:
> > Same as above, if you stick to your separation we won't even need to install
> > flake8 in the environments where only the tests are run.
> 
> Again, this is the environment for the tests (not for linting), and since this
> is a flake8 plugin, of course we need flake8 to test it.

I wonder how i could have missed this.. you are absolutely right!

LGTM

[Vasily Kuznetsov, 2017-10-10 12:58:16]

On 2017/10/10 05:34:30, Sebastian Noack wrote:
> https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini
> File flake8-eyeo/tox.ini (right):
> 
>
https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
> flake8-eyeo/tox.ini:2: envlist = py{27,35,36}-flake8{2,3},flake8
> On 2017/10/09 23:39:38, tlucas wrote:
> > In case you decide to stick to your separation of testing and linting, we
> won't
> > need additional environments for flake82 / flake83 (since flake8 won't be
run
> > anyway)
> 
> Well, this is the flake8-eyeo extension. The py{27,35,36}-flake8{2,3}
> environments don't lint this code, but run the tests. And obviously we have to
> test the extension with all supported versions of Python and flake8. We can
drop
> support for flake8 2 now, but this is out of the scope of this change.
> 
>
https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
> flake8-eyeo/tox.ini:8: deps =
> On 2017/10/09 23:39:38, tlucas wrote:
> > Same as above, if you stick to your separation we won't even need to install
> > flake8 in the environments where only the tests are run.
> 
> Again, this is the environment for the tests (not for linting), and since this
> is a flake8 plugin, of course we need flake8 to test it.
> 
>
https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
> flake8-eyeo/tox.ini:14: [testenv:flake8]
> On 2017/10/09 21:16:14, Vasily Kuznetsov wrote:
> > On 2017/10/09 02:08:58, Sebastian Noack wrote:
> > > This is somewhat unrelated, but it seems to make most sense for the tox
> setups
> > > where we have many environments, to lint our code separately, instead of
> each
> > > time for each Python version supported.
> > 
> > Indeed, it doesn't make sense to lint the code with the old version of
Flake8
> > since we don't care what it thinks if the new version likes the code.
> > 
> > However, as Flake8 is tied to the version of Python on which it runs [1], in
> > order to confirm that code is Python2-kosher and also Python3-kosher, we
might
> > need to run Flake8 on both Python2 and Python3. It's possible that the only
> > difference between running it on different interpreters is the ability to
> parse
> > Python3-syntax (irrelevant here since we can't use it), but I haven't found
> any
> > source that would confirm or disconfirm this, so if we want to be on the
safe
> > side, doing both seems advisable. Or maybe that's too paranoid... I'm not
> sure.
> > 
> > In any case it probably makes sense to select a specific version of Python
in
> > the linting environment to ensure consistency.
> > 
> > [1] http://flake8.pycqa.org/en/latest/ (see the note at the top of the page)
> 
> The note in the flake8 documentation merely reminds you that when you use
flake8
> on an older Python version, that it cannot parse (or at least properly
> understand) language features that have been added in newer versions of
Python.
> This, however, doesn't matter if your code is backwards compatible with Python
> 2, as you are stuck with the feature subset of all supported Python versions.
> 
> Regardless, I think it is sufficient to lint code that is compatible with
Python
> 3, on Python 3 only. If the code looks good in Python 3, it looks good for the
> future. And as long as it still runs on Python 2 (if required), I don't care
if
> in rare cases it might cause some additional linter warnings there. At some
> point, support for Python 2 will be dropped, anyway.

Yeah, I guess you're right, who cares about Python2-goodness if it's
Python3-good.

> It seems, by default, tox is using the latest available Python interpreter, if
> not otherwise specified. So IMO we are all good.

For me it's using 2.7 even though I have 3.6. I think it by default Tox uses the
Python that was used to run Tox, so pinning makes sense.

[Vasily Kuznetsov, 2017-10-10 13:00:08]

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini
File flake8-eyeo/tox.ini (right):

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
flake8-eyeo/tox.ini:16: flake8
Nit: these lines have different indentation level. Not that it matters for
anything but aesthetics, but would be neater if they were also indented by 4
spaces.

[Sebastian Noack, 2017-10-10 18:17:40]

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini
File flake8-eyeo/tox.ini (right):

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
flake8-eyeo/tox.ini:14: [testenv:flake8]
On 2017/10/10 12:58:16, Vasily Kuznetsov wrote:
> On 2017/10/10 05:34:30, Sebastian Noack wrote:
> > It seems, by default, tox is using the latest available Python interpreter,
if
> > not otherwise specified. So IMO we are all good.
> 
> For me it's using 2.7 even though I have 3.6. I think it by default Tox uses
the
> Python that was used to run Tox, so pinning makes sense.

Indeed, in my /usr/bin/tox it says #!/usr/bin/python3. But yeah, if you would
install tox with pip (not pip3 or apt), which I suspect is how you installed
tox, it might use Python 2 inside the venvs by default. Anyway, I added
basepython now.

https://codereview.adblockplus.org/29569634/diff/29569635/flake8-eyeo/tox.ini...
flake8-eyeo/tox.ini:16: flake8
On 2017/10/10 13:00:08, Vasily Kuznetsov wrote:
> Nit: these lines have different indentation level. Not that it matters for
> anything but aesthetics, but would be neater if they were also indented by 4
> spaces.

Done.

[Sebastian Noack, 2017-10-11 03:30:44]

https://codereview.adblockplus.org/29569634/diff/29573174/patchconv/tox.ini
File patchconv/tox.ini (right):

https://codereview.adblockplus.org/29569634/diff/29573174/patchconv/tox.ini#n...
patchconv/tox.ini:17: skip_install = true
Since this environment is only for linting, there is no reason to install the
package in here. (However, in case of flake8-eyeo it's different, since
flake8-eyeo should be linted with flake8-eyeo.)

[Vasily Kuznetsov, 2017-10-11 16:25:47]

LGTM