[PATCH] Add flake8 and isort to .pre-commit-config.yaml

[PATCH] Add flake8 and isort to .pre-commit-config.yaml

[Tom Tromey]

This adds flake8 and isort to .pre-commit-config.yaml.  This way, they
will automatically be run on commit.

I chose the most recent available versions after verifying that they
don't cause any reports or changes in the current tree.

Internally at AdaCore, we also use a few flake8 plugins as well, so
perhaps that's another avenue for investigation.
---
 .pre-commit-config.yaml | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 7afe60c20be..bca8acc5d02 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -6,3 +6,14 @@ repos:
     hooks:
       - id: black
         files: 'gdb/.*'
+  - repo:  https://github.com/pycqa/flake8
+    rev: 7.0.0
+    hooks:
+    - id: flake8
+      files: gdb/python/.*\.py$
+      args: [--config, gdb/setup.cfg]
+  - repo: https://github.com/pycqa/isort
+    rev: 5.13.2
+    hooks:
+    - id: isort
+      files: gdb/.*\.py$
-- 
2.43.0

Re: [PATCH] Add flake8 and isort to .pre-commit-config.yaml

[Simon Marchi]

On 2024-04-02 14:11, Tom Tromey wrote:
> This adds flake8 and isort to .pre-commit-config.yaml.  This way, they
> will automatically be run on commit.
> 
> I chose the most recent available versions after verifying that they
> don't cause any reports or changes in the current tree.
> 
> Internally at AdaCore, we also use a few flake8 plugins as well, so
> perhaps that's another avenue for investigation.
> ---
>  .pre-commit-config.yaml | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
> index 7afe60c20be..bca8acc5d02 100644
> --- a/.pre-commit-config.yaml
> +++ b/.pre-commit-config.yaml
> @@ -6,3 +6,14 @@ repos:
>      hooks:
>        - id: black
>          files: 'gdb/.*'
> +  - repo:  https://github.com/pycqa/flake8
> +    rev: 7.0.0
> +    hooks:
> +    - id: flake8
> +      files: gdb/python/.*\.py$
> +      args: [--config, gdb/setup.cfg]
> +  - repo: https://github.com/pycqa/isort
> +    rev: 5.13.2
> +    hooks:
> +    - id: isort
> +      files: gdb/.*\.py$

I only used `gdb/.*.py` for the black hook, because pre-commit does its
own filtering already.  The hooks specify the `python` type, so
pre-commit only runs the hook on what it believes is a Python file.  The
only purpose of the regex was to limit the hooks to the gdb
sub-directory.

However, I realized we have at least one file that is python but isn't
named *.py, gdb-gdb.py.in.  I just tried, and the black hook doesn't run
on gdb-gdb.py-in.  If you run black manually, it considers gdb-gdb.py.in
as a Python file (I don't know by which magic) and re-formats it:

    $ pwd
    /home/simark/src/binutils-gdb
    $ black gdb
    reformatted /home/simark/src/binutils-gdb/gdb/gdb-gdb.py.in

But the filtering done by pre-commit doesn't consider gdb-gdb.py.in a
Python file.  The hooks specify `types: 'python'`, which doesn't match
'*.py.in'.  The doc for `types` is here:

    https://pre-commit.com/#filtering-files-with-types

The mapping between extensions and types is here:

    https://github.com/pre-commit/identify/blob/main/identify/extensions.py

If we want to make pre-commit consider that file for our Python hooks,
the only solution I see currently would be to use something like this,
as described in the doc above:

    hooks:
      - id: black
        types_or: [file]
        files: '^gdb/.*\.(py|py.in)$'

It didn't seem to work for me using `types` (an error in gdb-gdb.py.in
wouldn't be caught), but it works using `types_or`.  So, I would
consider something similar for other hooks as well.

Simon

Re: [PATCH] Add flake8 and isort to .pre-commit-config.yaml

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> If we want to make pre-commit consider that file for our Python hooks,
Simon> the only solution I see currently would be to use something like this,
Simon> as described in the doc above:

I did this in v2.  Let me know what you think.

Tom

commit c3c3740f73b5f20697074c5a20a26d0805afbb98
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Apr 2 12:04:21 2024 -0600

    Add flake8 and isort to .pre-commit-config.yaml
    
    This adds flake8 and isort to .pre-commit-config.yaml.  This way, they
    will automatically be run on commit.
    
    I chose the most recent available versions after verifying that they
    don't cause any reports or changes in the current tree.
    
    Internally at AdaCore, we also use a few flake8 plugins as well, so
    perhaps that's another avenue for investigation.
    
    v2: Also update the various file-selection clauses to pick up
    gdb-gdb.py.in; include the isort change made to this file; and finally
    add a comment about the exclusions from flake8.

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 7afe60c20be..8721dac678b 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -5,4 +5,20 @@ repos:
     rev: 24.3.0
     hooks:
       - id: black
-        files: 'gdb/.*'
+        types_or: [file]
+        files: 'gdb/.*\.py(\.in)?$'
+  - repo:  https://github.com/pycqa/flake8
+    rev: 7.0.0
+    hooks:
+    - id: flake8
+      types_or: [file]
+      # Note this one is only run on gdb/python, not (for now) the
+      # test suite.
+      files: 'gdb/python/.*\.py(\.in)?$'
+      args: [--config, gdb/setup.cfg]
+  - repo: https://github.com/pycqa/isort
+    rev: 5.13.2
+    hooks:
+    - id: isort
+      types_or: [file]
+      files: 'gdb/.*\.py(\.in)?$'
diff --git a/gdb/gdb-gdb.py.in b/gdb/gdb-gdb.py.in
index 54db9b00cf3..b5a7fa4f390 100644
--- a/gdb/gdb-gdb.py.in
+++ b/gdb/gdb-gdb.py.in
@@ -15,9 +15,10 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-import gdb
 import os.path
 
+import gdb
+
 
 class TypeFlag:
     """A class that allows us to store a flag name, its short name,

Re: [PATCH] Add flake8 and isort to .pre-commit-config.yaml

[Simon Marchi]

On 4/3/24 3:52 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> If we want to make pre-commit consider that file for our Python hooks,
> Simon> the only solution I see currently would be to use something like this,
> Simon> as described in the doc above:
> 
> I did this in v2.  Let me know what you think.
> 
> Tom
> 
> commit c3c3740f73b5f20697074c5a20a26d0805afbb98
> Author: Tom Tromey <tromey@adacore.com>
> Date:   Tue Apr 2 12:04:21 2024 -0600
> 
>     Add flake8 and isort to .pre-commit-config.yaml
>     
>     This adds flake8 and isort to .pre-commit-config.yaml.  This way, they
>     will automatically be run on commit.
>     
>     I chose the most recent available versions after verifying that they
>     don't cause any reports or changes in the current tree.
>     
>     Internally at AdaCore, we also use a few flake8 plugins as well, so
>     perhaps that's another avenue for investigation.
>     
>     v2: Also update the various file-selection clauses to pick up
>     gdb-gdb.py.in; include the isort change made to this file; and finally
>     add a comment about the exclusions from flake8.
> 
> diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
> index 7afe60c20be..8721dac678b 100644
> --- a/.pre-commit-config.yaml
> +++ b/.pre-commit-config.yaml
> @@ -5,4 +5,20 @@ repos:
>      rev: 24.3.0
>      hooks:
>        - id: black
> -        files: 'gdb/.*'
> +        types_or: [file]
> +        files: 'gdb/.*\.py(\.in)?$'
> +  - repo:  https://github.com/pycqa/flake8
> +    rev: 7.0.0
> +    hooks:
> +    - id: flake8
> +      types_or: [file]
> +      # Note this one is only run on gdb/python, not (for now) the
> +      # test suite.
> +      files: 'gdb/python/.*\.py(\.in)?$'
> +      args: [--config, gdb/setup.cfg]
> +  - repo: https://github.com/pycqa/isort
> +    rev: 5.13.2
> +    hooks:
> +    - id: isort
> +      types_or: [file]
> +      files: 'gdb/.*\.py(\.in)?$'
> diff --git a/gdb/gdb-gdb.py.in b/gdb/gdb-gdb.py.in
> index 54db9b00cf3..b5a7fa4f390 100644
> --- a/gdb/gdb-gdb.py.in
> +++ b/gdb/gdb-gdb.py.in
> @@ -15,9 +15,10 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> -import gdb
>  import os.path
>  
> +import gdb
> +
>  
>  class TypeFlag:
>      """A class that allows us to store a flag name, its short name,

Thanks, LGTM.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH] Add flake8 and isort to .pre-commit-config.yaml

[Tom de Vries]

On 4/3/24 21:57, Simon Marchi wrote:
> On 4/3/24 3:52 PM, Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
>>
>> Simon> If we want to make pre-commit consider that file for our Python hooks,
>> Simon> the only solution I see currently would be to use something like this,
>> Simon> as described in the doc above:
>>
>> I did this in v2.  Let me know what you think.
>>
>> Tom
>>
>> commit c3c3740f73b5f20697074c5a20a26d0805afbb98
>> Author: Tom Tromey <tromey@adacore.com>
>> Date:   Tue Apr 2 12:04:21 2024 -0600
>>
>>      Add flake8 and isort to .pre-commit-config.yaml
>>      
>>      This adds flake8 and isort to .pre-commit-config.yaml.  This way, they
>>      will automatically be run on commit.
>>      
>>      I chose the most recent available versions after verifying that they
>>      don't cause any reports or changes in the current tree.
>>      
>>      Internally at AdaCore, we also use a few flake8 plugins as well, so
>>      perhaps that's another avenue for investigation.
>>      
>>      v2: Also update the various file-selection clauses to pick up
>>      gdb-gdb.py.in; include the isort change made to this file; and finally
>>      add a comment about the exclusions from flake8.
>>
>> diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
>> index 7afe60c20be..8721dac678b 100644
>> --- a/.pre-commit-config.yaml
>> +++ b/.pre-commit-config.yaml
>> @@ -5,4 +5,20 @@ repos:
>>       rev: 24.3.0
>>       hooks:
>>         - id: black
>> -        files: 'gdb/.*'
>> +        types_or: [file]
>> +        files: 'gdb/.*\.py(\.in)?$'
>> +  - repo:  https://github.com/pycqa/flake8
>> +    rev: 7.0.0
>> +    hooks:
>> +    - id: flake8
>> +      types_or: [file]
>> +      # Note this one is only run on gdb/python, not (for now) the
>> +      # test suite.
>> +      files: 'gdb/python/.*\.py(\.in)?$'
>> +      args: [--config, gdb/setup.cfg]
>> +  - repo: https://github.com/pycqa/isort
>> +    rev: 5.13.2
>> +    hooks:
>> +    - id: isort
>> +      types_or: [file]
>> +      files: 'gdb/.*\.py(\.in)?$'
>> diff --git a/gdb/gdb-gdb.py.in b/gdb/gdb-gdb.py.in
>> index 54db9b00cf3..b5a7fa4f390 100644
>> --- a/gdb/gdb-gdb.py.in
>> +++ b/gdb/gdb-gdb.py.in
>> @@ -15,9 +15,10 @@
>>   # You should have received a copy of the GNU General Public License
>>   # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>   
>> -import gdb
>>   import os.path
>>   
>> +import gdb
>> +
>>   
>>   class TypeFlag:
>>       """A class that allows us to store a flag name, its short name,
> 
> Thanks, LGTM.
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

Hi,

just to report back on this with my current state.

I use openleap 15.4 with python 3.6, and the used version of flake8 
requires python 3.7, so I ran into trouble after this commit.

Then I learned about "git commit -n", and started using that as 
workaround, but eventually ran into trouble because that doesn't seem to 
work while rebasing.

I then decided to install python 3.11 and use a virtual environment, and 
use git commit from within that environment.  AFAICT, the virtual 
environment works as expected:
...
$ python --version
Python 3.11.5
$ python3 --version
Python 3.11.5
...
but for some reason I keep getting:
...
       SyntaxError: future feature annotations is not defined
...
which is the same error I got with python 3.6.

I've tried removing ~/.cache/pre-commit a couple of times, but that 
didn't help either.

So, atm I can no longer rebase.  I'll try to workaround this by adding a 
local commit that reverts this change, or something similar, but if 
anybody has another idea I'd be happy to hear it.

Thanks,
- Tom

Re: [PATCH] Add flake8 and isort to .pre-commit-config.yaml

[Kévin Le Gouguec]

Tom de Vries <tdevries@suse.de> writes:

> just to report back on this with my current state.
>
> I use openleap 15.4 with python 3.6, and the used version of flake8 requires python 3.7, so I ran into trouble after this commit.
>
> Then I learned about "git commit -n", and started using that as workaround, but eventually ran into trouble because that doesn't seem to work while rebasing.
>
> I then decided to install python 3.11 and use a virtual environment, and use git commit from within that environment.  AFAICT, the virtual environment works as expected:
> ...
> $ python --version
> Python 3.11.5
> $ python3 --version
> Python 3.11.5
> ...
> but for some reason I keep getting:
> ...
>       SyntaxError: future feature annotations is not defined
> ...
> which is the same error I got with python 3.6.
>
> I've tried removing ~/.cache/pre-commit a couple of times, but that didn't help either.
>
> So, atm I can no longer rebase.  I'll try to workaround this by adding a local commit that reverts this change, or something similar, but if anybody has another idea I'd be happy to hear it.

Hunch: have you installed pre-commit in this virtual environment, or are
you using a pre-commit that comes from "outside", e.g. your distro, or
'pip install --user' before activating the venv?  FWIW this might yield
clues:

$ head -1 $(which pre-commit)

Over here this shows a shebang that points to […VENV…]/bin/python3
(which in turn is a symlink to my system Python).

I would hope that 'pip install pre-commit' within your virtual
environment would install a pre-commit that will use "the right Python",
but I could be missing something.

Re: [PATCH] Add flake8 and isort to .pre-commit-config.yaml

[Tom de Vries]

On 4/9/24 11:41, Kévin Le Gouguec wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> just to report back on this with my current state.
>>
>> I use openleap 15.4 with python 3.6, and the used version of flake8 requires python 3.7, so I ran into trouble after this commit.
>>
>> Then I learned about "git commit -n", and started using that as workaround, but eventually ran into trouble because that doesn't seem to work while rebasing.
>>
>> I then decided to install python 3.11 and use a virtual environment, and use git commit from within that environment.  AFAICT, the virtual environment works as expected:
>> ...
>> $ python --version
>> Python 3.11.5
>> $ python3 --version
>> Python 3.11.5
>> ...
>> but for some reason I keep getting:
>> ...
>>        SyntaxError: future feature annotations is not defined
>> ...
>> which is the same error I got with python 3.6.
>>
>> I've tried removing ~/.cache/pre-commit a couple of times, but that didn't help either.
>>
>> So, atm I can no longer rebase.  I'll try to workaround this by adding a local commit that reverts this change, or something similar, but if anybody has another idea I'd be happy to hear it.
> 
> Hunch: have you installed pre-commit in this virtual environment, or are
> you using a pre-commit that comes from "outside", e.g. your distro, or
> 'pip install --user' before activating the venv?  FWIW this might yield
> clues:
> 
> $ head -1 $(which pre-commit)
> 

Hi Kevin,

thanks for the reaction.

The command you mention showed the correct pre-commit version.

But when running git commit, the wrong one was used.  I just needed to 
rerun "pre-commit install" to re-install the hooks, which fixes this.

I'll update the wiki entry to clarify this.

Thanks,
- Tom

> Over here this shows a shebang that points to […VENV…]/bin/python3
> (which in turn is a symlink to my system Python).
> 
> I would hope that 'pip install pre-commit' within your virtual
> environment would install a pre-commit that will use "the right Python",
> but I could be missing something.

Re: [PATCH] Add flake8 and isort to .pre-commit-config.yaml

[Kévin Le Gouguec]

Tom de Vries <tdevries@suse.de> writes:

> But when running git commit, the wrong one was used.  I just needed to rerun "pre-commit install" to re-install the hooks, which fixes this.

Oooh, hadn't though of that - pre-commit does capture an
"INSTALL_PYTHON" into .git/hooks/pre-commit.  Good catch!

Re: [PATCH] Add flake8 and isort to .pre-commit-config.yaml

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> So, atm I can no longer rebase.  I'll try to workaround this by adding
Tom> a local commit that reverts this change, or something similar, but if
Tom> anybody has another idea I'd be happy to hear it.

It sounds like you found a fix, but in extremis you can also
"pre-commit uninstall" and just not use the hook.

Tom

Re: [PATCH] Add flake8 and isort to .pre-commit-config.yaml

[Tom de Vries]

On 4/9/24 17:22, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> So, atm I can no longer rebase.  I'll try to workaround this by adding
> Tom> a local commit that reverts this change, or something similar, but if
> Tom> anybody has another idea I'd be happy to hear it.
> 
> It sounds like you found a fix, but in extremis you can also
> "pre-commit uninstall" and just not use the hook.

Yes, I didn't realize that at the point I ran into the trouble.  Perhaps 
pre-commit could be improved to advertise considering using SKIP at that 
point, that could have helped.

I dropped the approach of installing a second python version alongside, 
I probably did something wrong but versions got mixed and things went 
downhill from there pretty quickly.  Deinstalling the second python 
version was easy, but manually purging various files in ~/.local and 
~/.cache to get back to a working state took some work.

Anyway, my current approach is to install the hooks and add a line:
...
export SKIP=flake8,isort
...

That way, I'm still running black.

Thanks,
- Tom