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:

Status badges

Description (last modified by Matthias Köppe)

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 Tobias Diez

Status: newneeds_review

comment:2 Changed 2 years ago by Matthias Köppe

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 do pycodestyle-minimal, not pycodestyle. This affects the default when -e is not used.

comment:3 Changed 2 years ago by Matthias Köppe

Type: PLEASE CHANGEenhancement

comment:4 Changed 2 years ago by Jean-Philippe Labbé

Cc: Jean-Philippe Labbé added

comment:5 Changed 2 years ago by git

Commit: d65a0273a87e1f61b7b40b35d07d2ef82ab20d9f6f4cfb5c44a2adaf328a7e289b03e7a18df2cfac

Branch pushed to git repo; I updated commit sha1. New commits:

6f4cfb5Implement feedback

comment:6 Changed 2 years ago by Tobias Diez

Thanks for the review! I've now implemented your suggestions.

comment:7 Changed 2 years ago by Matthias Köppe

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 Changed 2 years ago by Matthias Köppe

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 Matthias Köppe

Status: needs_reviewneeds_work

comment:10 in reply to:  7 Changed 2 years ago by Tobias Diez

Replying to mkoeppe:

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

You are probably right, but that's not really related to this ticket.

comment:11 in reply to:  8 ; Changed 2 years ago by Tobias Diez

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 Tobias Diez

Status: needs_workneeds_review

comment:13 in reply to:  11 ; Changed 2 years ago by Matthias Köppe

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 git

Commit: 6f4cfb5c44a2adaf328a7e289b03e7a18df2cfaca0d9b590b243182a7e461f8b01f9a10707546257

Branch pushed to git repo; I updated commit sha1. New commits:

064b8c6Merge tag '9.3.beta3' into t/31004/public/build/minimal_codestyle
a0d9b59src/bin/sage --advanced: Show all tox environments, adjust alignment

comment:15 in reply to:  8 ; Changed 2 years ago by Matthias Köppe

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 Matthias Köppe

Authors: Tobias DiezTobias Diez, Matthias Koeppe
Description: modified (diff)
Reviewers: Matthias Koeppe, ...
Summary: Add full codestyle as recommendationsrc/Add full codestyle as recommendation

comment:17 Changed 2 years ago by Matthias Köppe

Cc: Frédéric Chapoton added
Summary: src/Add full codestyle as recommendationsrc/tox.ini: Rename pycodestyle to pycodestyle-minimal, add full pycodestyle as recommendation

comment:18 in reply to:  15 Changed 2 years ago by Tobias Diez

Replying to mkoeppe:

Replying to mkoeppe:

Also, the help message shown by sage -advanced needs some cosmetic adjustments because the longest environment name is longer now

Done

Thanks!

comment:19 in reply to:  13 Changed 2 years ago by Tobias Diez

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 Matthias Köppe

Let's investigate this in #31049. It's not related to this ticket.

comment:21 Changed 2 years ago by Matthias Köppe

Ready for review...

comment:22 Changed 2 years ago by Matthias Köppe

Is there a reviewer for my changes on this ticket?

comment:23 Changed 2 years ago by Tobias Diez

Reviewers: Matthias Koeppe, ...Matthias Koeppe, Tobias Diez
Status: needs_reviewpositive_review

The other changes look good to me as well. Thanks!

comment:24 Changed 2 years ago by Volker Braun

Branch: public/build/minimal_codestylea0d9b590b243182a7e461f8b01f9a10707546257
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.