nextstrain / augur

Pipeline components for real-time phylodynamic analysis
https://docs.nextstrain.org/projects/augur/
GNU Affero General Public License v3.0
268 stars 128 forks source link

Review Pyright rule exceptions #1472

Open victorlin opened 6 months ago

victorlin commented 6 months ago

Pyright was added in #1246. The default Pyright configuration options result in many violations from Augur's code as-is. To allow incremental adoption of Pyright, all default rule violations were disabled.

Tasks

Go through each of these and either (1) decide to keep the exception or (2) remove the exception and fix all violations in the code.

victorlin commented 6 months ago

From @jameshadfield in https://github.com/nextstrain/augur/pull/1246#discussion_r1604241775:

I looked at a few of these - totally fine if we want to defer any and all to separate PRs beyond this one:

  • reportUnusedExpression - single failure, should fix
  • reportUndefinedVariable - single failure, already with a flake8 ignore comment
  • reportPrivateImportUsage - no failures
  • reportGeneralTypeIssues - 9 errors, but all seem worth fixing
victorlin commented 6 months ago

reportPrivateImportUsage - no failures

I didn't get any errors locally but did in CI (noted in e5ced1c8b5e1aeb1923bc00219594336e8ceeb34):

2024-05-17T00:31:30.1458148Z tests/test_pyright.py::test_pyright /home/runner/work/augur/augur/augur/clades.py
2024-05-17T00:31:30.1466206Z   /home/runner/work/augur/augur/augur/clades.py:328:18 - error: "read" is not exported from module "Bio.Phylo" (reportPrivateImportUsage)
…
2024-05-17T00:31:30.1553703Z 20 errors, 0 warnings, 0 informations 
2024-05-17T00:31:30.3631414Z FAILED

I now realize that it's specific to the Biopython version. Pyright passed in CI with biopython=1.80 but failed on latest (1.83).

victorlin commented 3 weeks ago

Alternatively, we can adopt a baseline approach which applies the rules to new/modified code. Seems good for our use case.