Opened 2 years ago

Closed 2 years ago

#30361 closed enhancement (fixed)

Add pyright config and linting documentation

Reported by: Tobias Diez Owned by:
Priority: minor Milestone: sage-9.3
Component: build Keywords:
Cc: Matthias Köppe, Frédéric Chapoton, Michael Jung, 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 Samuel Lelièvre)

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 2 years ago by Tobias Diez

Branch: public/manifolds/pyrightpublic/mainfolds/pyright
Commit: 6ab9fe9e9ad650ef8cf6dd0f148d5370f68d5572

New commits:

6ab9fe9Add pyright config

comment:2 Changed 2 years ago by Samuel Lelièvre

Description: modified (diff)

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

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 2 years ago by Tobias Diez

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

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

comment:6 Changed 2 years ago by git

Commit: 6ab9fe9e9ad650ef8cf6dd0f148d5370f68d5572256a9f5f9ed735045008f0ea99e2d41250ee6755

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

256a9f5Extend configuration

comment:7 Changed 2 years ago by Tobias Diez

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 2 years ago by Tobias Diez

Status: newneeds_review

comment:9 Changed 2 years ago by git

Commit: 256a9f5f9ed735045008f0ea99e2d41250ee6755dc11c2682fbe174c273391f85de16aca6f96c7a4

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

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

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

Cc: Michael Jung added

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

comment:11 Changed 2 years ago by git

Commit: dc11c2682fbe174c273391f85de16aca6f96c7a41f876389ac2af8eb2fca554585657c49c2405840

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 2 years ago by Tobias Diez

Done, anything else to do?

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

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

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

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 2 years ago by git

Commit: 1f876389ac2af8eb2fca554585657c49c240584086e21f3c7acbcaaaa5009ed9d3282b9b81c4dd3e

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 2 years ago by Tobias Diez

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

Summary: Add pyright configAdd pyright config and linting documentation

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

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

Dependencies: #30408, #30410

comment:19 Changed 2 years ago by Tobias Diez

Dependencies: #30408, #30410#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 2 years ago by Matthias Köppe

Authors: Tobias Diez
Cc: gh-kliem added

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

pyflakes documentation needs more work

comment:22 Changed 2 years ago by Tobias Diez

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

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

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

Reviewers: Matthias Koeppe

comment:25 Changed 2 years ago by git

Commit: 86e21f3c7acbcaaaa5009ed9d3282b9b81c4dd3e88f24d00e8d95183a1d60ed7887b16e0fd7c37b1

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

88f24d0Remove dots

comment:26 Changed 2 years ago by Tobias Diez

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


New commits:

88f24d0Remove dots

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

Status: needs_reviewneeds_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 2 years ago by Tobias Diez

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

comment:29 Changed 2 years ago by Tobias Diez

Status: needs_workneeds_review

comment:30 Changed 2 years ago by git

Commit: 88f24d00e8d95183a1d60ed7887b16e0fd7c37b1524a2983ce89d74e7111aac1d57e8fbfc67caa81

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

524a298Revert changes in chart class

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

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

Status: needs_reviewneeds_work

comment:33 Changed 2 years ago by git

Commit: 524a2983ce89d74e7111aac1d57e8fbfc67caa814159c3aa530e4bdc4ff5faefb449761d952770a2

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

4159c3aFix indent

comment:34 Changed 2 years ago by Tobias Diez

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

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

Status: needs_reviewpositive_review

patchbot is green, positive review

comment:37 Changed 2 years ago by Dima Pasechnik

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 2 years ago by Tobias Diez

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 2 years ago by Volker Braun

Status: positive_reviewneeds_work

PDF docs don't build

comment:40 Changed 2 years ago by Dima Pasechnik

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 2 years ago by Tobias Diez

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

comment:42 Changed 2 years ago by git

Commit: 4159c3aa530e4bdc4ff5faefb449761d952770a27049f391cabc30574ec07e7f217774eaffcc09f5

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

7049f39add missing '_'s

comment:43 Changed 2 years ago by Dima Pasechnik

Last edited 2 years ago by Dima Pasechnik (previous) (diff)

comment:44 Changed 2 years ago by Tobias Diez

Thanks!

comment:45 Changed 2 years ago by Dima Pasechnik

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 2 years ago by Dima Pasechnik

Reviewers: Matthias KoeppeMatthias Koeppe, Dima Pasechnik
Status: needs_workpositive_review

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

comment:47 Changed 2 years ago by Tobias Diez

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

comment:48 Changed 2 years ago by Frédéric Chapoton

patience, please

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

Milestone: sage-9.2sage-9.3

comment:50 Changed 2 years ago by Volker Braun

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