8000 Run autotyping on entire codebase by vincentmacri · Pull Request #40180 · sagemath/sage · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Run autotyping on entire codebase #40180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

vincentmacri
Copy link
Contributor
@vincentmacri vincentmacri commented May 28, 2025

I used https://github.com/JelleZijlstra/autotyping/ to add type annotations for all magic methods and methods that return a basic Python type.

Inspired by @fchapoton's use of autoformatting tools to improve the Sage codebase.

Right now I've done two passes with autotyping. The first adds type annotations to all magic methods (__init__, __len__, etc.), the second adds type annotations to any method that always returns a basic Python literal (for instance, is_ methods that always return a bool, and _latex_ methods that always return a str).

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

@vincentmacri
Copy link
Contributor Author
vincentmacri commented May 28, 2025

I'm marking this as a draft because this is an unreasonably large PR, but I'm not sure how to best go about splitting it up. @fchapoton any thoughts?

Copy link
Contributor
@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look for "Return type" in https://github.com/sagemath/sage/actions/runs/15310228472/job/43072906204?pr=40180, you see that a few of the added annotations are not corresponding to what the code actually returns at the moment.

I got the impression that most of these are actual minor bugs that should be fixed (eg adding pass in abstract methods). Not sure though if all of these failures are due to code bugs, or if some of these methods are totally fine to return say int | None.

@@ -701,7 +701,7 @@ def _repr_term(self, m):
"""
p, v = m

def ppr(i):
def ppr(i) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/home/runner/work/sage/sage/src/sage/algebras/quantum_clifford.py:704:23 - warning: Function with declared return type "str" must return value on all code paths
Type "None" cannot be assigned to type "str" (reportGeneralTypeIssues)

@@ -766,7 +766,7 @@ class ElementMethods:
# return self == self.parent().zero()
< 8000 /td>
@abstract_method
def __bool__(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/home/runner/work/sage/sage/src/sage/categories/additive_magmas.py:769:35 - warning: Function with declared return type "bool" must return value on all code paths
Type "None" cannot be assigned to type "bool" (reportGeneralTypeIssues)

@@ -178,7 +178,7 @@ def dual_code(self):
return self
return super().dual_code()

def minimum_distance(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/home/runner/work/sage/sage/src/sage/coding/golay_code.py:181:35 - warning: Function with declared return type "int" must return value on all code paths
Type "None" cannot be assigned to type "int" (reportGeneralTypeIssues)

@vincentmacri
Copy link
Contributor Author

If you look for "Return type" in https://github.com/sagemath/sage/actions/runs/15310228472/job/43072906204?pr=40180, you see that a few of the added annotations are not corresponding to what the code actually returns at the moment.

I got the impression that most of these are actual minor bugs that should be fixed (eg adding pass in abstract methods). Not sure though if all of these failures are due to code bugs, or if some of these methods are totally fine to return say int | None.

Yeah, this is an automatic tool and I only gave a quick glance at the output, hence why the PR is marked as a draft. Given how many files this PR touches, do you think it needs to be split up, and if so, how should we split this up?

@tobiasdiez
Copy link
Contributor

In my opinion it would be okay to merge this in its current state (after fixing the trivial doctest failure). The existing typing info is currently not in a state where it would be really authoritative. So it's fine if some of the added typing annotations in this PR are wrong - the important part is that most of them are correct. (Maybe we should add a short paragraph in the dev docs about the current state of the typing info, and what to do in case the typing checker complains.)

For the future progress, I would propose to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0