Opened 2 years ago
Closed 2 years ago
#31004 closed enhancement (fixed)
src/tox.ini: Rename pycodestyle to pycodestyle-minimal, add full pycodestyle as recommendation
Reported by: | Tobias Diez | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.3 |
Component: | build | Keywords: | |
Cc: | Matthias Köppe, Jean-Philippe 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 pycodestyle-minimal
, 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: | Jean-Philippe 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 follow-up: 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 follow-ups: 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 follow-up: 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 tox-related 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/sage-version.sh: line 3: __requires__: command not found /mnt/d/Programming/sage/src/.venv/bin/sage-version.sh: line 4: syntax error near unexpected token `'pkg_resources'' /mnt/d/Programming/sage/src/.venv/bin/sage-version.sh: line 4: `__import__('pkg_resources').require('sage==9.3b2')'
comment:12 Changed 2 years ago by
Status: | needs_work → needs_review |
---|
comment:13 follow-up: 19 Changed 2 years ago by
Replying to gh-tobiasdiez:
I also get the following error when running
sage
:/mnt/d/Programming/sage/src/.venv/bin/sage-version.sh: line 3: __requires__: command not found /mnt/d/Programming/sage/src/.venv/bin/sage-version.sh: line 4: syntax error near unexpected token `'pkg_resources'' /mnt/d/Programming/sage/src/.venv/bin/sage-version.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 follow-up: 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 pycodestyle-minimal, add full pycodestyle as recommendation |
comment:18 Changed 2 years ago by
comment:19 Changed 2 years ago by
Replying to mkoeppe:
Replying to gh-tobiasdiez:
I also get the following error when running
sage
:/mnt/d/Programming/sage/src/.venv/bin/sage-version.sh: line 3: __requires__: command not found /mnt/d/Programming/sage/src/.venv/bin/sage-version.sh: line 4: syntax error near unexpected token `'pkg_resources'' /mnt/d/Programming/sage/src/.venv/bin/sage-version.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
pycodestyle-minimal
would be more tox-ic (tox factor conditions)2) I think
envlist
at the top should dopycodestyle-minimal
, notpycodestyle
. This affects the default when-e
is not used.