Support traversal of TypeAlias for Python 3.12 #115

ericbn · 2024-04-10T03:38:36Z

Also update tox and GitHub Actions to use Python 3.12 by default.

coveralls · 2024-04-12T19:13:33Z

Pull Request Test Coverage Report for Build 8745231098

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

82 of 83 (98.8%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.002%) to 98.261%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/ssort/_parsing.py	8	9	88.89%

Totals
Change from base Build 7702612545:	0.002%
Covered Lines:	1356
Relevant Lines:	1380

💛 - Coveralls

tests/test_requirements.py

ericbn

I've added a commit with the proposed fixes.

src/ssort/_parsing.py

ericbn · 2024-04-13T02:36:17Z

src/ssort/_requirements.py

+    if sys.version_info >= (3, 12):
+        for type_param in node.type_params:
+            yield from get_requirements(type_param)
+        scope.update(type_param.name for type_param in node.type_params)  # type: ignore[attr-defined]


I've added the # type: ignore[attr-defined] here an in a couple of lines below because mypy complained that "type_param" has no attribute "name". But type_param is TypeVar | ParamSpec | TypeVarTuple and all the latter have a name.

ericbn · 2024-04-13T02:37:48Z

src/ssort/_requirements.py

+            if not requirement.deferred:
+                requirement = dataclasses.replace(requirement, deferred=True)


I've copied the two lines above from the similar code in _get_requirements_for_function_def. I don't know what a referred requirement is or if this code is needed here.

This is good. Might be worth factoring out a _get_scope_from_type_params function.

A deferred requirement is one that is captured by a statement but not used immediately.

For example here, a has an immediate requirement on b:

a = b

and here the requirement is deferred:

def a(): return b

src/ssort/_requirements.py

bwhmather · 2024-04-13T06:39:09Z

tests/test_bindings.py

+@type_parameter_syntax
+def test_type_var_bindings():
+    node = _parse("type RecursiveList[T] = T | list[RecursiveList[T]]")
+    assert list(get_bindings(node)) == ["RecursiveList"]


I'm surprised that this all just worked without any changes to _bindings.py. Nice.

Yep. “Type aliases can refer to themselves” (as defined in the PEP) works because _get_requirements_for_type_alias does scope.add(node.name.id).

The only new binding introduced here is for TypeAliases, which have a Name and is resolved by the existing _get_bindings_for_name function. Type parameters, which are TypeVar | ParamSpec | TypeVarTuple, are treated for TypeAlias | FunctionDef | AsyncFunctionDef | ClassDef as arguments are treated for FunctionDef | AsyncFunctionDef | Lambda: they are added into the current scope for their usages to be resolved.

ericbn

Thank you for the review again. I've added a new commit with the latest suggestion, plus extracted a couple more functions.

ericbn · 2024-04-14T02:43:40Z

src/ssort/_parsing.py

-            if token.string == ")":
-                depth -= 1
+
+    def _optional_nested_brackets(open: str, close: str):


I've extracted the repeated code here into _optional_nested_brackets.

Would you mind hoisting it out of split_class?

I'm thinking of reverting this refactoring of _optional_nested_brackets and leaving a more complete refactoring of split_class for a latter PR. Agree?

Curious to see it. Go ahead.

ericbn · 2024-04-14T02:45:07Z

src/ssort/_requirements.py

@@ -32,6 +33,13 @@ def get_requirements(node: ast.AST) -> Iterable[Requirement]:
        yield from get_requirements(child)


+def _get_requirements_from_nodes(


I've hopefully made the code a little more simple with a _get_requirements_from_nodes function.

ericbn · 2024-04-14T02:50:33Z

src/ssort/_requirements.py

+        scope = _get_scope_from_type_params(node.type_params)
+        for requirement in _get_requirements_from_nodes(node.type_params):
+            if requirement.name not in scope:
+                yield requirement


It's now using the scope from type params to filter their own requirements. This adds the compliance to:

Type parameters are visible to other type parameters declared elsewhere in the list. This allows type parameters to use other type parameters within their definition.

for the PEP. It's not considering any order for sake of simplicity, leaving the more strict rules to the compiler or type checkers.

Yup, had read that as scope being updated from left to right but I think you are right.

Also extract _get_requirements_from_nodes, _get_scope_from_type_params and _optional_nested_brackets functions.

bwhmather · 2024-04-18T07:13:39Z

src/ssort/_requirements.py


-    scope = set(CLASS_BUILTINS)
+    for requirement in _get_requirements_from_nodes(node.bases):
+        if requirement.name not in scope:


bwhmather · 2024-04-18T07:19:03Z

I think this is just about done. Should have time this evening to do a full read through then will merge and bake a release.

bwhmather

No more comments from me. Looks good. Hoist or revert _optional_nested_brackets and will merge when next at the keyboard.

ericbn · 2024-04-18T22:09:44Z

Cool! I've reverted _optional_nested_brackets. The reason I didn't want to hoist it was because I thought it would have a weird signature, as if depends on both token and tokens, and mutates token:

    token = _optional_nested_brackets(token, tokens, "[", "]")
    token = _optional_nested_brackets(token, tokens, "(", ")")

Instead, I think this would be better refactored as a having split_class become a class and _optional_nested_brackets a private method, with token and tokens being instance attributes. I tried a similar effect first by having _optional_nested_brackets as a closure.

bwhmather

Thank you. Merging and releasing.

Support traversal of TypeAlias for Python 3.12

e38c013

Also update tox and GitHub Actions to use Python 3.12 by default.

ericbn mentioned this pull request Apr 10, 2024

Support Python 3.12 PEP 695 - Type Parameter Syntax #102

Closed

bwhmather reviewed Apr 12, 2024

View reviewed changes

tests/test_requirements.py Outdated Show resolved Hide resolved

bwhmather reviewed Apr 12, 2024

View reviewed changes

tests/test_requirements.py Show resolved Hide resolved

Add requirements processing for type parameter syntax

6e765f4

ericbn force-pushed the add-type-alias-support branch from c1c52f5 to 6e765f4 Compare April 13, 2024 02:31

ericbn commented Apr 13, 2024

View reviewed changes

bwhmather reviewed Apr 13, 2024

View reviewed changes

src/ssort/_requirements.py Outdated Show resolved Hide resolved

bwhmather reviewed Apr 13, 2024

View reviewed changes

ericbn force-pushed the add-type-alias-support branch from 43c277b to d062f04 Compare April 14, 2024 02:42

ericbn commented Apr 14, 2024

View reviewed changes

Fix type parameter scopes

ac899c9

Also extract _get_requirements_from_nodes, _get_scope_from_type_params and _optional_nested_brackets functions.

ericbn force-pushed the add-type-alias-support branch from d062f04 to ac899c9 Compare April 14, 2024 02:59

bwhmather reviewed Apr 18, 2024

View reviewed changes

Revert refactoring of _optional_nested_brackets

20b3f63

bwhmather approved these changes Apr 19, 2024

View reviewed changes

bwhmather enabled auto-merge April 19, 2024 05:44

bwhmather merged commit 2025243 into bwhmather:master Apr 19, 2024
22 checks passed

ericbn deleted the add-type-alias-support branch April 19, 2024 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support traversal of TypeAlias for Python 3.12 #115

Support traversal of TypeAlias for Python 3.12 #115

ericbn commented Apr 10, 2024

coveralls commented Apr 12, 2024 •

edited

Loading

ericbn left a comment

ericbn Apr 13, 2024

ericbn Apr 13, 2024

bwhmather Apr 13, 2024

bwhmather Apr 13, 2024

bwhmather Apr 13, 2024

ericbn Apr 13, 2024

ericbn Apr 17, 2024

ericbn left a comment

ericbn Apr 14, 2024

bwhmather Apr 18, 2024

ericbn Apr 18, 2024

bwhmather Apr 18, 2024

ericbn Apr 14, 2024

ericbn Apr 14, 2024

bwhmather Apr 18, 2024

bwhmather Apr 18, 2024

bwhmather commented Apr 18, 2024

bwhmather left a comment

ericbn commented Apr 18, 2024

bwhmather left a comment

		if not requirement.deferred:
		requirement = dataclasses.replace(requirement, deferred=True)

		@@ -32,6 +33,13 @@ def get_requirements(node: ast.AST) -> Iterable[Requirement]:
		yield from get_requirements(child)


		def _get_requirements_from_nodes(

Support traversal of TypeAlias for Python 3.12 #115

Support traversal of TypeAlias for Python 3.12 #115

Conversation

ericbn commented Apr 10, 2024

coveralls commented Apr 12, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8745231098

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

ericbn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericbn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwhmather commented Apr 18, 2024

bwhmather left a comment

Choose a reason for hiding this comment

ericbn commented Apr 18, 2024

bwhmather left a comment

Choose a reason for hiding this comment

coveralls commented Apr 12, 2024 •

edited

Loading