Skip to content

PEP8 and typing annotations for function fields#40972

Merged
vbraun merged 17 commits intosagemath:developfrom
vincentmacri:function-fields
Oct 16, 2025
Merged

PEP8 and typing annotations for function fields#40972
vbraun merged 17 commits intosagemath:developfrom
vincentmacri:function-fields

Conversation

@vincentmacri
Copy link
Copy Markdown
Member

@vincentmacri vincentmacri commented Oct 4, 2025

Mostly done with ruff and autotyping, but also some manually for the main function field classes.

📝 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.

⌛ Dependencies

@vincentmacri vincentmacri changed the title Code cleanup and typing annotations for function fields PEP8 and typing annotations for function fields Oct 4, 2025
@vincentmacri vincentmacri requested a review from kryzar October 4, 2025 00:29
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 4, 2025

Documentation preview for this PR (built with commit 09e0016; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Copy link
Copy Markdown
Contributor

@fchapoton fchapoton left a comment

Choose a reason for hiding this comment

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

ok, but please next time:

  • do no make any change about spaces around arithmetic operators ;
  • rather do only annotations changes, when changing so many files
  • I would also prefer to avoid Literal for the moment

@vincentmacri
Copy link
Copy Markdown
Member Author

vincentmacri commented Oct 6, 2025

Thanks for the review!

* do no make any change about spaces around arithmetic operators ;

I can certainly do that, but can I ask why you don't like the spaces around arithmetic operators? Do you not like spaces around operators, or is it just too annoying to review for how minor of a fix it is?

* rather do only annotations changes, when changing so many files

For sure! Originally I was not planning to change this many files but I guess I got carried away.

* I would also prefer to avoid Literal for the moment

Sure. There aren't many places where we can use it anyway since it only works with Python literals. Something like Literal[Integer(1)] doesn't work. So the only places where Literal could theoretically be used are the handful of functions that use int instead of Integer, functions that return a constant bool, or maybe for things like an algorithm parameter that has a handful of valid string options.

The reason I wanted to use Literal was I was hoping that Cython would be smart enough that something like this would be optimized out, but after some testing it seems Cython is not capable of this (at least not yet, maybe one day):

def constant_false() -> Literal[False]:
    return False

The hope was that if a Cython file imported constant_false from a Python module and then tried something like this:

if constant_false():
    assert False

that the if statement would be optimized out. Unfortunately, at least as far as I can tell, it isn't currently optimized out.

@vincentmacri
Copy link
Copy Markdown
Member Author

@fchapoton Just resolving the merge conflict. Still good?

Copy link
Copy Markdown
Contributor

@fchapoton fchapoton left a comment

Choose a reason for hiding this comment

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

ok

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 12, 2025
sagemathgh-40972: PEP8 and typing annotations for function fields
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Mostly done with `ruff` and `autotyping`, but also some manually for the
main function field classes.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] 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.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#40972
Reported by: Vincent Macri
Reviewer(s): Frédéric Chapoton
@vbraun vbraun merged commit 146a8ec into sagemath:develop Oct 16, 2025
24 of 25 checks passed
@vincentmacri vincentmacri deleted the function-fields branch February 26, 2026 20:59
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.

3 participants