Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src/tox.ini: Rename pycodestyle to pycodestyle-minimal, add full pycodestyle as recommendation #31004

Closed
tobiasdiez opened this issue Dec 4, 2020 · 23 comments

Comments

@tobiasdiez
Copy link
Contributor

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.

CC: @mkoeppe @jplab @fchapoton

Component: build

Author: Tobias Diez, Matthias Koeppe

Branch/Commit: a0d9b59

Reviewer: Matthias Koeppe, Tobias Diez

Issue created by migration from https://trac.sagemath.org/ticket/31004

@tobiasdiez tobiasdiez added this to the sage-9.3 milestone Dec 4, 2020
@mkoeppe
Copy link
Member

mkoeppe commented Dec 4, 2020

comment:2

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 5, 2020

Changed commit from d65a027 to 6f4cfb5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 5, 2020

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

6f4cfb5Implement feedback

@tobiasdiez
Copy link
Contributor Author

comment:6

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

@mkoeppe
Copy link
Member

mkoeppe commented Dec 5, 2020

comment:7

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

@mkoeppe
Copy link
Member

mkoeppe commented Dec 5, 2020

comment:8

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

@tobiasdiez
Copy link
Contributor Author

comment:10

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.

@tobiasdiez
Copy link
Contributor Author

comment:11

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')'

@mkoeppe
Copy link
Member

mkoeppe commented Dec 7, 2020

comment:13

Replying to @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?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2020

Changed commit from 6f4cfb5 to a0d9b59

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2020

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

@mkoeppe
Copy link
Member

mkoeppe commented Dec 7, 2020

comment:15

Replying to @mkoeppe:

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

Done

@mkoeppe
Copy link
Member

mkoeppe commented Dec 7, 2020

Reviewer: Matthias Koeppe, ...

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 7, 2020

Changed author from Tobias Diez to Tobias Diez, Matthias Koeppe

@mkoeppe mkoeppe changed the title Add full codestyle as recommendation src/Add full codestyle as recommendation Dec 7, 2020
@mkoeppe mkoeppe changed the title src/Add full codestyle as recommendation src/tox.ini: Rename pycodestyle to pycodestyle-minimal, add full pycodestyle as recommendation Dec 7, 2020
@tobiasdiez
Copy link
Contributor Author

comment:18

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!

@tobiasdiez
Copy link
Contributor Author

comment:19

Replying to @mkoeppe:

Replying to @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)?

@mkoeppe
Copy link
Member

mkoeppe commented Dec 14, 2020

comment:20

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

@mkoeppe
Copy link
Member

mkoeppe commented Dec 14, 2020

comment:21

Ready for review...

@mkoeppe
Copy link
Member

mkoeppe commented Dec 22, 2020

comment:22

Is there a reviewer for my changes on this ticket?

@tobiasdiez
Copy link
Contributor Author

comment:23

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

@tobiasdiez
Copy link
Contributor Author

Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Tobias Diez

@vbraun
Copy link
Member

vbraun commented Jan 17, 2021

Changed branch from public/build/minimal_codestyle to a0d9b59

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants