src/tox.ini: Check patchbot plugin patterns and common typo patterns

[mkoeppe]

(from #30448)

We add a tox environment that checks all of the static patterns that the patchbot checks - https://github.com/sagemath/sage-patchbot/blob/master/sage_patchbot/plugins.py#L601

This is so that developers can check style locally instead of going through endless cycles with the patchbot on trac.

This uses https://pypi.org/project/relint/

Example:

   sage -tox -e relint src/sage/plot/

Depends on #30410

CC: @fchapoton @jhpalmieri @tobiasdiez @tscrim @slel @kliem

Component: scripts

Author: Matthias Koeppe

Branch/Commit: 7cf9efe

Reviewer: Jonathan Kliem, Tobias Diez

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

[mkoeppe]

Branch: u/mkoeppe/src_tox_ini__check_patchbot_plugin_patterns

[sagetrac-git]

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

4a3a33a	Add comment
b0ad03e	src/bin/sage: Show tox environment list in 'sage -advanced'

[sagetrac-git]

Commit: b0ad03e

[mkoeppe]

Author: Matthias Koeppe

[tobiasdiez]

comment:10

Overall looks good to me. A few suggestions:

• Please try not to use sh -c in the tox, because this is not platform compatible i.e. doesn't work on Windows.
• The trailing whitespace error can also be checked by pycodestyle, https://pycodestyle.pycqa.org/en/latest/intro.html#error-codes W291. Thus, I guess this custom regex lint rule can be replaced by adding W291 to the pycodestyle config.
• Not sure if the first "python 3 incompatible" check is still needed. Since if you use python 3 incompatible code, then it simply shouldn't run, right?
• relint should also be documented Add pyright config and linting documentation #30361 and added to the lint action Add pyright and pycodestyle checks as Github actions #30404

[mkoeppe]

comment:11

Replying to @tobiasdiez:

Overall looks good to me. A few suggestions:

• Please try not to use sh -c in the tox, because this is not platform compatible i.e. doesn't work on Windows.

Thanks for the heads-up. Do you have a specific suggestion what to do instead?

• The trailing whitespace error can also be checked by pycodestyle, https://pycodestyle.pycqa.org/en/latest/intro.html#error-codes W291. Thus, I guess this custom regex lint rule can be replaced by adding W291 to the pycodestyle config.
• Not sure if the first "python 3 incompatible" check is still needed. Since if you use python 3 incompatible code, then it simply shouldn't run, right?

Right. For now I just wanted to get these on par with the patchbot. I would prefer to make further refinements in follow-up tickets.

Regarding Python 3 - these patterns may still be helpful when reviving ancient branches. Some of these patterns, of course, will be flagged by the Python 3 parser; but other patterns are for syntactically correct but outdated library functions that may be used in parts of code that is not covered by doctests.

• relint should also be documented Add pyright config and linting documentation #30361

I'll do this in #30453 (Document "sage -tox")

and added to the lint action #30404

#30404 should possibly just invoke tox

[tobiasdiez]

comment:12

I think in most cases you can simply remove the sh -c part. The if/for statements need a bit more work, probably the easiest solution is to write a small python wrapper.

Scrolling through the tox documentation, I'm also not sure if the processing of arguments is really in the spirit of tox. For example, the foreach loop for relint makes it impossible to pass further cmd args to relint (since they would end up in the loop). On the other hand, I see that its also convenient to have a systematic interface to apply the command to only one file. Maybe leave this functionality to sage --tox?

[mkoeppe]

comment:13

Replying to @tobiasdiez:

Scrolling through the tox documentation, I'm also not sure if the processing of arguments is really in the spirit of tox. For example, the foreach loop for relint makes it impossible to pass further cmd args to relint (since they would end up in the loop). On the other hand, I see that its also convenient to have a systematic interface to apply the command to only one file. Maybe leave this functionality to sage --tox?

tox runs all environments by default -- so there needs to be a uniform interface for file/directory arguments

[mkoeppe]

comment:14

Replying to @tobiasdiez:

I think in most cases you can simply remove the sh -c part. The if/for statements need a bit more work, probably the easiest solution is to write a small python wrapper.

OK. I think I am going to change sage --coverage so that it can invoke sage --coverageall if necessary.

Then I can get rid of sh -c for all "sagedirect" environments.

[mkoeppe]

comment:15

I'll do this in #30474

[mkoeppe]

comment:16

Replying to @tobiasdiez:

... Maybe leave this functionality to sage --tox?

No, I think it's important to keep plain tox from the command line the same as sage --tox

[mkoeppe]

comment:17

Needs review.

[sagetrac-git]

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

7cf9efe

src/.relint.yml: Add pattern from #30472

[sagetrac-git]

Changed commit from b0ad03e to 7cf9efe

[kliem]

comment:21

I like it. It saves you from waiting on the patchbots to do trivial stuff.

As for the python3 style. That is still helpful. Iterator classes will probably still take .next(), but you shouldn't use it anymore and at some point, this should be removed.

Works for me.

@tobiasdiez: Do you have anything that needs changing yet.

[kliem]

Reviewer: Jonathan Kliem

[tobiasdiez]

comment:22

No, it looks fine for me as well (given that #30474 is addressed at some point).

[mkoeppe]

Changed reviewer from Jonathan Kliem to Jonathan Kliem, Tobias Diez

[mkoeppe]

comment:23

Thanks!

[vbraun]

Changed branch from u/mkoeppe/src_tox_ini__check_patchbot_plugin_patterns to 7cf9efe