Skip to content
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

Integrate from __future__ import annotations #4643

Merged
merged 1 commit into from
Nov 16, 2024

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Nov 13, 2024

A conservative implementation of integrating from __future__ import annotations in the codebase. Updates all instances where type hints could utilize new syntax, utilizing newly added Ruff rules to find & adjust said instances. The resulting type-hints are much cleaner, even granting functionality which otherwise require 3.9 or 3.10 as their minimum Python version:

# Before
def some_function(env: "SConsEnvironment") -> Optional[Tuple[List[str], Union[int, float]]]:

# After
def some_function(env: SConsEnvironment) -> tuple[list[str], int | float]] | None:

As a result of this new implementation, the hacky workarounds for forward-declaring types will no longer be necessary. Namely, sctyping.py is no longer necessary & has been removed. Instead, one should use from typing import TYPE_CHECKING & resolve imports like any other.


There's a more involved alternative of this PR that adds from __future__ import annotations as a required import instead, making it appear on ALL scripts in the codebase. While it's arguably a "better" option, it would also create significantly more noise in the changelog. If this approach were to be taken, it'd have to be part of a wider refactor.

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt and RELEASE.txt (and read the README.rst).
  • I have updated the appropriate documentation

@Repiteo Repiteo force-pushed the future-annotations branch 2 times, most recently from 2b4486c to c5735b3 Compare November 13, 2024 19:18
@mwichmann mwichmann added the maintenance Tasks to maintain internal SCons code/tools label Nov 13, 2024
@mwichmann
Copy link
Collaborator

Not sure we should ditch sctypes - be good to have a standard place for SCons-specific type combinations. For example, a ton of environment methods take an argument that's either a pathname string, or a file node, or a list of pathname strings and/or file nodes. That one is always going to be awkward to describe, even if we can stop using Union.

@mwichmann
Copy link
Collaborator

Not sure we should ditch sctypes - be good to have a standard place for SCons-specific type combinations. For example, a ton of environment methods take an argument that's either a pathname string, or a file node, or a list of pathname strings and/or file nodes. That one is always going to be awkward to describe, even if we can stop using Union.

oops, I conflated sctypes and sctyping. never mind....

@mwichmann
Copy link
Collaborator

It's a pain that after years of requests, GitHub still doesn't support some form of stacked PRs. I've tried some external solutions to this, and frankly, they were unusable for the kind of workflow SCons uses.

@mwichmann
Copy link
Collaborator

Looks good!

@bdbaddog
Copy link
Contributor

Good work. Thanks!

@bdbaddog bdbaddog merged commit 45a4d57 into SCons:master Nov 16, 2024
9 of 10 checks passed
@Repiteo Repiteo deleted the future-annotations branch November 16, 2024 23:52
@mwichmann mwichmann added this to the NextRelease milestone Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Tasks to maintain internal SCons code/tools
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

3 participants