Patchwork [7,of,7] test: add a basic 'test-check-pylint.t'

login
register
mail settings
Submitter Pierre-Yves David
Date March 15, 2017, 7:01 a.m.
Message ID <e1b0b1909c43858ff4b4.1489561279@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/19353/
State Accepted
Headers show

Comments

Pierre-Yves David - March 15, 2017, 7:01 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1482964970 -3600
#      Wed Dec 28 23:42:50 2016 +0100
# Node ID e1b0b1909c43858ff4b4e1821afbee9626c4cf0f
# Parent  b24451e8f9a6beee5501e9bedc47af7edc283ea4
# EXP-Topic check-pylint
# Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
#              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r e1b0b1909c43
test: add a basic 'test-check-pylint.t'

We add a minimal check using pylint for one case we knows we care about:
"mutable default" argument.

We'll likely extend this over time to cover other useful checks but this is a
good starting point.
Gregory Szorc - March 15, 2017, 4:35 p.m.
On Mar 15, 2017, at 07:56, Kevin Bullock <kbullock+mercurial@ringworld.org> wrote:

>> On Mar 15, 2017, at 02:01, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>> 
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1482964970 -3600
>> #      Wed Dec 28 23:42:50 2016 +0100
>> # Node ID e1b0b1909c43858ff4b4e1821afbee9626c4cf0f
>> # Parent  b24451e8f9a6beee5501e9bedc47af7edc283ea4
>> # EXP-Topic check-pylint
>> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r e1b0b1909c43
>> test: add a basic 'test-check-pylint.t'
>> 
>> We add a minimal check using pylint for one case we knows we care about:
>> "mutable default" argument.
>> 
>> We'll likely extend this over time to cover other useful checks but this is a
>> good starting point.
> 
> I had to dig a bit to convince myself that pyflakes, which we already integrated, won't catch this. It indeed will not, so this LGTM. Copy-edited the new test and pushed.

We may want to look into flake8. It wraps the popular linters in a single interface and is quite nice.

The downside for our use case is that output will be different unless all the linters it wraps are present.
Pierre-Yves David - March 16, 2017, 11:14 a.m.
On 03/15/2017 10:13 AM, Kevin Bullock wrote:
>> On Mar 15, 2017, at 11:35, Gregory Szorc <gregory.szorc@gmail.com> wrote:
>>
>> On Mar 15, 2017, at 07:56, Kevin Bullock <kbullock+mercurial@ringworld.org> wrote:
>>
>>>> On Mar 15, 2017, at 02:01, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>>>> # Date 1482964970 -3600
>>>> #      Wed Dec 28 23:42:50 2016 +0100
>>>> # Node ID e1b0b1909c43858ff4b4e1821afbee9626c4cf0f
>>>> # Parent  b24451e8f9a6beee5501e9bedc47af7edc283ea4
>>>> # EXP-Topic check-pylint
>>>> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>>> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r e1b0b1909c43
>>>> test: add a basic 'test-check-pylint.t'
>>>>
>>>> We add a minimal check using pylint for one case we knows we care about:
>>>> "mutable default" argument.
>>>>
>>>> We'll likely extend this over time to cover other useful checks but this is a
>>>> good starting point.
>>>
>>> I had to dig a bit to convince myself that pyflakes, which we already integrated, won't catch this. It indeed will not, so this LGTM. Copy-edited the new test and pushed.
>>
>> We may want to look into flake8. It wraps the popular linters in a single interface and is quite nice.
>>
>> The downside for our use case is that output will be different unless all the linters it wraps are present.
>
> Will flake8 also run pylint? I thought it was only pep8 + pyflakes.
>
> There are also a number of ways our style differs from PEP8, so that piece in particular wouldn't get us much over this one, where we now have both test-check-pyflakes.t and test-check-pylint.t. Worth considering, though, in the (very likely) case others know more than I do about flake8's capabilities.

flake8 is run on the evolve extension (I think credit goes to Sean for 
the initial idea). Our style is quite far away from PEP8 so I've 
disabled a fair amount of message, but it still catch many useful thing.

* evolve flake8 tests: 
https://www.mercurial-scm.org/repo/evolve/file/tip/tests/test-check-flake8.t

* evolve flake8 config: 
https://www.mercurial-scm.org/repo/evolve/file/tip/setup.cfg

Cheers,
Sean Farley - March 22, 2017, 5:19 a.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 03/15/2017 10:13 AM, Kevin Bullock wrote:
>>> On Mar 15, 2017, at 11:35, Gregory Szorc <gregory.szorc@gmail.com> wrote:
>>>
>>> On Mar 15, 2017, at 07:56, Kevin Bullock <kbullock+mercurial@ringworld.org> wrote:
>>>
>>>>> On Mar 15, 2017, at 02:01, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>>>>
>>>>> # HG changeset patch
>>>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>>>>> # Date 1482964970 -3600
>>>>> #      Wed Dec 28 23:42:50 2016 +0100
>>>>> # Node ID e1b0b1909c43858ff4b4e1821afbee9626c4cf0f
>>>>> # Parent  b24451e8f9a6beee5501e9bedc47af7edc283ea4
>>>>> # EXP-Topic check-pylint
>>>>> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>>>> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r e1b0b1909c43
>>>>> test: add a basic 'test-check-pylint.t'
>>>>>
>>>>> We add a minimal check using pylint for one case we knows we care about:
>>>>> "mutable default" argument.
>>>>>
>>>>> We'll likely extend this over time to cover other useful checks but this is a
>>>>> good starting point.
>>>>
>>>> I had to dig a bit to convince myself that pyflakes, which we already integrated, won't catch this. It indeed will not, so this LGTM. Copy-edited the new test and pushed.
>>>
>>> We may want to look into flake8. It wraps the popular linters in a single interface and is quite nice.
>>>
>>> The downside for our use case is that output will be different unless all the linters it wraps are present.
>>
>> Will flake8 also run pylint? I thought it was only pep8 + pyflakes.
>>
>> There are also a number of ways our style differs from PEP8, so that piece in particular wouldn't get us much over this one, where we now have both test-check-pyflakes.t and test-check-pylint.t. Worth considering, though, in the (very likely) case others know more than I do about flake8's capabilities.
>
> flake8 is run on the evolve extension (I think credit goes to Sean for 
> the initial idea). Our style is quite far away from PEP8 so I've 
> disabled a fair amount of message, but it still catch many useful thing.
>
> * evolve flake8 tests: 
> https://www.mercurial-scm.org/repo/evolve/file/tip/tests/test-check-flake8.t
>
> * evolve flake8 config: 
> https://www.mercurial-scm.org/repo/evolve/file/tip/setup.cfg

Yep! I was planning on adding some config to core Mercurial one day. I
*think* one config file can be used for pylint + flake8. It's low on my
priority list, though, so don't hold your breath. Also, it's fairly
straight-forward so maybe a new student could tackle this?

Patch

diff --git a/tests/hghave.py b/tests/hghave.py
--- a/tests/hghave.py
+++ b/tests/hghave.py
@@ -413,6 +413,12 @@  def has_pyflakes():
                        br"<stdin>:1: 're' imported but unused",
                        True)
 
+@check("pylint", "Pylint python linter")
+def has_pylint():
+    return matchoutput("pylint --help",
+                       br"Usage:  pylint",
+                       True)
+
 @check("pygments", "Pygments source highlighting library")
 def has_pygments():
     try:
diff --git a/tests/test-check-pylint.t b/tests/test-check-pylint.t
new file mode 100644
--- /dev/null
+++ b/tests/test-check-pylint.t
@@ -0,0 +1,12 @@ 
+#require test-repo pylint hg10
+
+Run pylint for known rules we care about.
+-----------------------------------------
+
+There should be no recorded failure, fix the codebase before introducing a new checks.
+
+Current check:
+- W0102: no mutable default argument
+
+  $ touch $TESTTMP/fakerc
+  $ pylint --rcfile=$TESTTMP/fakerc --disable=all --enable=W0102 --reports=no mercurial hgext hgext3rd