Opened 2 years ago
Closed 2 years ago
#31004 closed enhancement (fixed)
src/tox.ini: Rename pycodestyle to pycodestyleminimal, add full pycodestyle as recommendation
Reported by:  Tobias Diez  Owned by:  

Priority:  major  Milestone:  sage9.3 
Component:  build  Keywords:  
Cc:  Matthias Köppe, JeanPhilippe Labbé, Frédéric Chapoton  Merged in:  
Authors:  Tobias Diez, Matthias Koeppe  Reviewers:  Matthias Koeppe, Tobias Diez 
Report Upstream:  N/A  Work issues:  
Branch:  a0d9b59 (Commits, GitHub, GitLab)  Commit:  a0d9b590b243182a7e461f8b01f9a10707546257 
Dependencies:  Stopgaps: 
Description (last modified by )
The current pycodestyle configuration only covers a few rules that are also enforced by the patchbot. While this is a good starting point, IDEs pick up this configuration and only show warnings for these rules. In order to make sage's code adhere more to the pep8 recommendations, it is desirable to get warnings for all code style errors. Thus, in this ticket, we enable all pycodestyle warnings. These can be tested as usually by calling pycodestyle
or tox e pycodestyle
. Moreover, the current minimal ruleset is tested via the new environment tox e pycodestyleminimal
, which is also used in the new lint github action workflow.
Change History (24)
comment:1 Changed 2 years ago by
Status:  new → needs_review 

comment:2 Changed 2 years ago by
comment:3 Changed 2 years ago by
Type:  PLEASE CHANGE → enhancement 

comment:4 Changed 2 years ago by
Cc:  JeanPhilippe Labbé added 

comment:5 Changed 2 years ago by
Commit:  d65a0273a87e1f61b7b40b35d07d2ef82ab20d9f → 6f4cfb5c44a2adaf328a7e289b03e7a18df2cfac 

Branch pushed to git repo; I updated commit sha1. New commits:
6f4cfb5  Implement feedback

comment:7 followup: 10 Changed 2 years ago by
In lint.yml
, I think you don't need to install relint
or pycodestyle
because tox will take care of that in its virtual environment
comment:8 followups: 11 15 Changed 2 years ago by
Also, the help message shown by sage advanced
needs some cosmetic adjustments because the longest environment name is longer now
comment:9 Changed 2 years ago by
Status:  needs_review → needs_work 

comment:10 Changed 2 years ago by
Replying to mkoeppe:
In
lint.yml
, I think you don't need to installrelint
orpycodestyle
because tox will take care of that in its virtual environment
You are probably right, but that's not really related to this ticket.
comment:11 followup: 13 Changed 2 years ago by
Replying to mkoeppe:
Also, the help message shown by
sage advanced
needs some cosmetic adjustments because the longest environment name is longer now
I'm not sure what you mean, but sage advanced
doesn't show any toxrelated infos for me (but the indent for example ecl is off). Maybe it's because I also get the following error when running sage
:
/mnt/d/Programming/sage/src/.venv/bin/sageversion.sh: line 3: __requires__: command not found /mnt/d/Programming/sage/src/.venv/bin/sageversion.sh: line 4: syntax error near unexpected token `'pkg_resources'' /mnt/d/Programming/sage/src/.venv/bin/sageversion.sh: line 4: `__import__('pkg_resources').require('sage==9.3b2')'
comment:12 Changed 2 years ago by
Status:  needs_work → needs_review 

comment:13 followup: 19 Changed 2 years ago by
Replying to ghtobiasdiez:
I also get the following error when running
sage
:/mnt/d/Programming/sage/src/.venv/bin/sageversion.sh: line 3: __requires__: command not found /mnt/d/Programming/sage/src/.venv/bin/sageversion.sh: line 4: syntax error near unexpected token `'pkg_resources'' /mnt/d/Programming/sage/src/.venv/bin/sageversion.sh: line 4: `__import__('pkg_resources').require('sage==9.3b2')'
This is an interesting error  something seems to be rewriting this shell scripts as if it is a Python script  perhaps the newest version of pip?
comment:14 Changed 2 years ago by
Commit:  6f4cfb5c44a2adaf328a7e289b03e7a18df2cfac → a0d9b590b243182a7e461f8b01f9a10707546257 

comment:15 followup: 18 Changed 2 years ago by
Replying to mkoeppe:
Also, the help message shown by
sage advanced
needs some cosmetic adjustments because the longest environment name is longer now
Done
comment:16 Changed 2 years ago by
Authors:  Tobias Diez → Tobias Diez, Matthias Koeppe 

Description:  modified (diff) 
Reviewers:  → Matthias Koeppe, ... 
Summary:  Add full codestyle as recommendation → src/Add full codestyle as recommendation 
comment:17 Changed 2 years ago by
Cc:  Frédéric Chapoton added 

Summary:  src/Add full codestyle as recommendation → src/tox.ini: Rename pycodestyle to pycodestyleminimal, add full pycodestyle as recommendation 
comment:18 Changed 2 years ago by
comment:19 Changed 2 years ago by
Replying to mkoeppe:
Replying to ghtobiasdiez:
I also get the following error when running
sage
:/mnt/d/Programming/sage/src/.venv/bin/sageversion.sh: line 3: __requires__: command not found /mnt/d/Programming/sage/src/.venv/bin/sageversion.sh: line 4: syntax error near unexpected token `'pkg_resources'' /mnt/d/Programming/sage/src/.venv/bin/sageversion.sh: line 4: `__import__('pkg_resources').require('sage==9.3b2')'This is an interesting error  something seems to be rewriting this shell scripts as if it is a Python script  perhaps the newest version of pip?
Maybe because I invoke sage
from within pipenv shell
(to activate the correct venv)?
comment:20 Changed 2 years ago by
Let's investigate this in #31049. It's not related to this ticket.
comment:23 Changed 2 years ago by
Reviewers:  Matthias Koeppe, ... → Matthias Koeppe, Tobias Diez 

Status:  needs_review → positive_review 
The other changes look good to me as well. Thanks!
comment:24 Changed 2 years ago by
Branch:  public/build/minimal_codestyle → a0d9b590b243182a7e461f8b01f9a10707546257 

Resolution:  → fixed 
Status:  positive_review → closed 
This is a good idea. Two points:
1) I think using
pycodestyleminimal
would be more toxic (tox factor conditions)2) I think
envlist
at the top should dopycodestyleminimal
, notpycodestyle
. This affects the default whene
is not used.