#30467 closed enhancement (fixed)

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

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.2
Component: scripts Keywords:
Cc: chapoton, jhpalmieri, gh-tobiasdiez, tscrim, slelievre, gh-kliem Merged in:
Authors: Matthias Koeppe Reviewers: Jonathan Kliem, Tobias Diez
Report Upstream: N/A Work issues:
Branch: 7cf9efe (Commits, GitHub, GitLab) Commit: 7cf9efed6dd7702fc0c3df6f487dd3ddab260bb0
Dependencies: #30410 Stopgaps:

Status badges

Description (last modified by 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/

Change History (24)

comment:1 Changed 14 months ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 14 months ago by mkoeppe

  • Description modified (diff)

comment:3 Changed 14 months ago by mkoeppe

  • Description modified (diff)

comment:4 Changed 14 months ago by mkoeppe

  • Description modified (diff)

comment:5 Changed 14 months ago by mkoeppe

  • Branch set to u/mkoeppe/src_tox_ini__check_patchbot_plugin_patterns

comment:6 Changed 14 months ago by git

  • Commit set to b0ad03e0932500900a64793c8ddb9c8fe2b2d4cf

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

4a3a33aAdd comment
b0ad03esrc/bin/sage: Show tox environment list in 'sage -advanced'

comment:7 Changed 14 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Cc chapoton jhpalmieri gh-tobiasdiez added
  • Status changed from new to needs_review

comment:8 Changed 14 months ago by mkoeppe

  • Description modified (diff)

comment:9 Changed 14 months ago by mkoeppe

  • Cc tscrim added

comment:10 follow-up: Changed 14 months ago by gh-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.
  • 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 #30361 and added to the lint action #30404

comment:11 in reply to: ↑ 10 Changed 14 months ago by mkoeppe

Replying to gh-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 #30361

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

and added to the lint action #30404

#30404 should possibly just invoke tox

comment:12 follow-ups: Changed 14 months ago by gh-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.

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?

Last edited 14 months ago by gh-tobiasdiez (previous) (diff)

comment:13 in reply to: ↑ 12 Changed 14 months ago by mkoeppe

Replying to gh-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

comment:14 in reply to: ↑ 12 Changed 14 months ago by mkoeppe

Replying to gh-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.

comment:15 Changed 14 months ago by mkoeppe

I'll do this in #30474

comment:16 in reply to: ↑ 12 Changed 14 months ago by mkoeppe

Replying to gh-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

comment:17 Changed 14 months ago by mkoeppe

Needs review.

comment:18 Changed 14 months ago by git

  • Commit changed from b0ad03e0932500900a64793c8ddb9c8fe2b2d4cf to 7cf9efed6dd7702fc0c3df6f487dd3ddab260bb0

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

7cf9efesrc/.relint.yml: Add pattern from #30472

comment:19 Changed 14 months ago by mkoeppe

  • Cc slelievre added
  • Summary changed from src/tox.ini: Check patchbot plugin patterns to src/tox.ini: Check patchbot plugin patterns and common typo patterns

comment:20 Changed 14 months ago by mkoeppe

  • Cc gh-kliem added

comment:21 Changed 14 months ago by gh-kliem

  • Reviewers set to Jonathan Kliem

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.

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

comment:22 Changed 14 months ago by gh-tobiasdiez

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

comment:23 Changed 14 months ago by mkoeppe

  • Reviewers changed from Jonathan Kliem to Jonathan Kliem, Tobias Diez
  • Status changed from needs_review to positive_review

Thanks!

comment:24 Changed 13 months ago by vbraun

  • Branch changed from u/mkoeppe/src_tox_ini__check_patchbot_plugin_patterns to 7cf9efed6dd7702fc0c3df6f487dd3ddab260bb0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.