#30361 closed enhancement (fixed)

Add pyright config and linting documentation

Reported by: gh-tobiasdiez Owned by:
Priority: minor Milestone: sage-9.3
Component: build Keywords:
Cc: mkoeppe, chapoton, gh-mjungmath, gh-kliem Merged in:
Authors: Tobias Diez Reviewers: Matthias Koeppe, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 7049f39 (Commits, GitHub, GitLab) Commit: 7049f391cabc30574ec07e7f217774eaffcc09f5
Dependencies: #30408 Stopgaps:

Status badges

Description (last modified by slelievre)

This PR provides a minimal configuration for the static typing checker pyright. In #29775 typing for the manifolds package is added. As this is currently the only place where typing is used, it's the only source folder for pyright.

There are also quite a few other code check options. These seem to cover similar rules as pyflakes and pycodestyle. Since these two checker have problems with typings (at least in the versions that they are currently used in the patchbot), it might be worthwhile to investigate if they can be completely replaced by pyright.

Change History (50)

comment:1 Changed 14 months ago by gh-tobiasdiez

  • Branch changed from public/manifolds/pyright to public/mainfolds/pyright
  • Commit set to 6ab9fe9e9ad650ef8cf6dd0f148d5370f68d5572

New commits:

6ab9fe9Add pyright config

comment:2 Changed 14 months ago by slelievre

  • Description modified (diff)

comment:3 Changed 14 months ago by mkoeppe

It would be nice to be able to replicate the style that the current patchbot plugins enforce using these other tools. That would allow developers to form an informed opinion on what tools we should be focusing on...

comment:4 Changed 14 months ago by gh-tobiasdiez

Pyright is in the first place a static type checker, something that pyflakes does not do (or at least is not designed to do).

Not that this PR only adds the config file, which aids the developer if he chooses to use pyright locally. It doesn't say that it needs to be used, or is integrated in the build workflow. That can be decided later. Given that pyright relies on node and has superior performance over alternatives as myphy, I could imagine that its good to be used in the build workflow and by developers that use VS code. Python-puristic developers propbably prefer a checker that can be installed via pip.

So I would propose to keep this PR as simple as possible, just add the config file for those people that want to use pyright, and add alternatives / integration with the build workflow later.

comment:5 Changed 14 months ago by mkoeppe

Could you include a comment at the top of this config file with a pointer to documentation or something?

comment:6 Changed 14 months ago by git

  • Commit changed from 6ab9fe9e9ad650ef8cf6dd0f148d5370f68d5572 to 256a9f5f9ed735045008f0ea99e2d41250ee6755

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

256a9f5Extend configuration

comment:7 Changed 14 months ago by gh-tobiasdiez

It's not possible to add comments to json files. But from the name of the file it should be rather clear that it belongs to pyright.

I've extended the pyright config a bit. There are no errors (for the manifolds part), but about 200 warnings. Some of them are false positives due to issues of pyright with cython / lazy import (reported as https://github.com/microsoft/pyright/issues/954 and https://github.com/microsoft/pyright/issues/952), but mostly they are valid things that should be fixed (e.g. possibly unbound variables). If you run pyright across the whole sage code, then there are 400 errors and 20k warnings.

I've also fixed one small issue where a getter was used instead of a setter.

comment:8 Changed 14 months ago by gh-tobiasdiez

  • Status changed from new to needs_review

comment:9 Changed 14 months ago by git

  • Commit changed from 256a9f5f9ed735045008f0ea99e2d41250ee6755 to dc11c2682fbe174c273391f85de16aca6f96c7a4

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

dc11c26src/doc/en/developer/tools.rst: Add stub

comment:10 Changed 14 months ago by mkoeppe

  • Cc gh-mjungmath added

How about adding a little section in the developer's guide?

comment:11 Changed 14 months ago by git

  • Commit changed from dc11c2682fbe174c273391f85de16aca6f96c7a4 to 1f876389ac2af8eb2fca554585657c49c2405840

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

a04823fAdd documentation
1f87638Merge branch 'public/mainfolds/pyright' of git://trac.sagemath.org/sage into public/mainfolds/pyright

comment:12 Changed 14 months ago by gh-tobiasdiez

Done, anything else to do?

comment:13 Changed 14 months ago by mkoeppe

Could you move this new text to the section that I created? src/doc/en/developer/tools.rst?

comment:14 Changed 14 months ago by mkoeppe

Also I was hoping for more hands-on documentation -- show what users need to install & type rather than just sending people to the tool's documentation

comment:15 Changed 14 months ago by git

  • Commit changed from 1f876389ac2af8eb2fca554585657c49c2405840 to 86e21f3c7acbcaaaa5009ed9d3282b9b81c4dd3e

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

2a54d7cMerge branch 'develop' of git://github.com/sagemath/sage into public/mainfolds/pyright
86e21f3Improve documentation

comment:16 Changed 14 months ago by gh-tobiasdiez

Ohh, sorry, I've overlooked that you've already created a new file for it. Thanks! I've now expanded the documentation.

comment:17 Changed 14 months ago by mkoeppe

  • Summary changed from Add pyright config to Add pyright config and linting documentation

Please take a look at #30410 as a possible interface to be advertised in the documentation.

comment:18 Changed 14 months ago by mkoeppe

  • Dependencies set to #30408, #30410

comment:19 Changed 14 months ago by gh-tobiasdiez

  • Dependencies changed from #30408, #30410 to #30408

I think it's a good idea to have a consistent interface for running linters and other checks . Thus I welcome the idea of #30410. However, this is rather unrelated to this ticket so I reversed the dependencies.

comment:20 Changed 14 months ago by mkoeppe

  • Authors set to Tobias Diez
  • Cc gh-kliem added

comment:21 Changed 14 months ago by mkoeppe

pyflakes documentation needs more work

comment:22 Changed 14 months ago by gh-tobiasdiez

Yeah, I agree but the aim of this ticket wasn't to document the whole lint and testing infrastructure of sage ;-). So let's do this at #30415.

comment:23 Changed 14 months ago by mkoeppe

If you just remove the "......" and the docbuild passes without errors, this is a positive review

comment:24 Changed 14 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe

comment:25 Changed 14 months ago by git

  • Commit changed from 86e21f3c7acbcaaaa5009ed9d3282b9b81c4dd3e to 88f24d00e8d95183a1d60ed7887b16e0fd7c37b1

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

88f24d0Remove dots

comment:26 Changed 14 months ago by gh-tobiasdiez

I've removed the dots (previously I thought they have to be there in restructuredtext ;-))


New commits:

88f24d0Remove dots

comment:27 Changed 14 months ago by mkoeppe

  • Status changed from needs_review to needs_work
[dochtml] [manifolds]   File "/Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.7/site-packages/matplotlib/sphinxext/plot_directive.py", line 472, in run_code
[dochtml] [manifolds]     exec(code, ns)
[dochtml] [manifolds]   File "<string>", line 11, in <module>
[dochtml] [manifolds]   File "/Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.7/site-packages/sage/misc/decorators.py", line 492, in wrapper
[dochtml] [manifolds]     return func(*args, **options)
[dochtml] [manifolds]   File "/Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.7/site-packages/sage/manifolds/chart.py", line 2931, in plot
[dochtml] [manifolds]     resu.set_aspect_ratio(1)
[dochtml] [manifolds] AttributeError: 'Graphics3dGroup' object has no attribute 'set_aspect_ratio'
[dochtml] [manifolds] The inventory files are in local/share/doc/sage/inventory/en/reference/manifolds.

comment:28 Changed 14 months ago by gh-tobiasdiez

Good that you found this mistake. I've now reverted my changes to the chart.py file.

comment:29 Changed 14 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

comment:30 Changed 14 months ago by git

  • Commit changed from 88f24d00e8d95183a1d60ed7887b16e0fd7c37b1 to 524a2983ce89d74e7111aac1d57e8fbfc67caa81

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

524a298Revert changes in chart class

comment:31 Changed 14 months ago by mkoeppe

[dochtml] OSError: /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/src/doc/en/developer/tools.rst:17: WARNING: Unexpected indentation.
[dochtml] 

comment:32 Changed 14 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:33 Changed 14 months ago by git

  • Commit changed from 524a2983ce89d74e7111aac1d57e8fbfc67caa81 to 4159c3aa530e4bdc4ff5faefb449761d952770a2

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

4159c3aFix indent

comment:34 Changed 14 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

The indention should be fixed now. At least I hope so, as I couldn't find any documentation on how to test/validate rst files.

comment:35 Changed 14 months ago by mkoeppe

The only way to validate it is to build the documentation using make doc-html...

It would actually be great to have a quick validation which we could plug into tox

comment:36 Changed 14 months ago by mkoeppe

  • Status changed from needs_review to positive_review

patchbot is green, positive review

comment:37 Changed 14 months ago by dimpase

Some types info has been added in a new code in recent tickets by gh-Ivo-Maffei, e.g. in #30337

Does this mean that pyrightconfig.json has to be modified accordingly?

comment:38 Changed 14 months ago by gh-tobiasdiez

Since pyright is currently not used in the patchbot (or something similar), the configuration file is only important for people that use pyright locally in their development. I would thus suggest that as soon as a bit of typing information has been added in a certain package, than this package should be added to the pyright include statement.

The long term goal should be to set pyrights rules to strict and have all of sage's code tested for each code contribution. I'm not sure what's the most practical way to achieve this.

comment:39 Changed 14 months ago by vbraun

  • Status changed from positive_review to needs_work

PDF docs don't build

comment:40 Changed 14 months ago by dimpase

The error is

[docpdf] ! You can't use `macro parameter character #' in math mode.
[docpdf] l.7363 ...n <https://github.com/microsoft/pyright#
[docpdf]                                                   installation>\) for details.
[docpdf] ? 
[docpdf] ! Emergency stop.

comment:41 Changed 14 months ago by gh-tobiasdiez

Ok, but what is wrong with this link? Should I remove the #installation part?

comment:42 Changed 14 months ago by git

  • Commit changed from 4159c3aa530e4bdc4ff5faefb449761d952770a2 to 7049f391cabc30574ec07e7f217774eaffcc09f5

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

7049f39add missing '_'s

comment:43 Changed 14 months ago by dimpase

Last edited 14 months ago by dimpase (previous) (diff)

comment:44 Changed 14 months ago by gh-tobiasdiez

Thanks!

comment:45 Changed 14 months ago by dimpase

I'm still checking that the pdf docs build (it requires a more complete install of TexLive? than I have, and so takes a while)

comment:46 Changed 14 months ago by dimpase

  • Reviewers changed from Matthias Koeppe to Matthias Koeppe, Dima Pasechnik
  • Status changed from needs_work to positive_review

OK, alles gut/goed, een kleine taaloefening voor \usepackage{babel}

comment:47 Changed 13 months ago by gh-tobiasdiez

Is there anything that needs to be done from my side for this to be merged?

comment:48 Changed 13 months ago by chapoton

patience, please

comment:49 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:50 Changed 11 months ago by vbraun

  • Branch changed from public/mainfolds/pyright to 7049f391cabc30574ec07e7f217774eaffcc09f5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.