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

test: refactor: use built-in collection types for type hints (Python 3.9 / PEP 585) #28725

Merged

Conversation

theStack
Copy link
Contributor

With Python 3.9 / PEP 585, type hinting has become a little less awkward, as for collection types one doesn't need to import the corresponding capitalized types (Dict, List, Set, Tuple, ...) anymore, but can use the built-in types directly (see https://peps.python.org/pep-0585/#implementation for the full list).

This PR applies the replacement for all Python scripts (i.e. in the contrib and test folders) for the basic types, i.e.:

  • typing.Dict -> dict
  • typing.List -> list
  • typing.Set -> set
  • typing.Tuple -> tuple

For an additional check, I ran mypy 1.6.1 on both master and the PR branch via

$ mypy --ignore-missing-imports --explicit-package-bases $(git ls-files "*.py")

and verified that the output is identical -- (from the 22 identified problems, most look like false-positives, it's probably worth it to go deeper here and address them in a follow-up though).

…585)

Since Python 3.9, type hinting has become a little less awkward, as for
collection types one doesn't need to import the corresponding
capitalized types (`Dict`, `List`, `Set`, `Tuple`, ...) anymore, but can
use the built-in types directly. [1] [2]
This commit applies the replacement for all Python scripts (i.e. in the
contrib and test folders) for the basic types:
    - typing.Dict  -> dict
    - typing.List  -> list
    - typing.Set   -> set
    - typing.Tuple -> tuple

[1] https://docs.python.org/3.9/whatsnew/3.9.html#type-hinting-generics-in-standard-collections
[2] https://peps.python.org/pep-0585/#implementation for a list of type
…y script

-BEGIN VERIFY SCRIPT-
sed -i 's|t\.Dict|dict|g'   ./contrib/verify-binaries/verify.py
sed -i 's|t\.List|list|g'   ./contrib/verify-binaries/verify.py
sed -i 's|t\.Tuple|tuple|g' ./contrib/verify-binaries/verify.py
-END VERIFY SCRIPT-
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 24, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK stickies-v, fanquake
Concept ACK kevkevinpal

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27114 (p2p: Allow whitelisting outgoing connections by brunoerg)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot added the Tests label Oct 24, 2023
@theStack theStack changed the title test: use built-in collection types for type hints (Python 3.9 / PEP 585) test: refactor: use built-in collection types for type hints (Python 3.9 / PEP 585) Oct 24, 2023
@kevkevinpal
Copy link
Contributor

running these two grep commands I got no results back which is good
grep -nr -e "Dict\[" -e "List\[" -e "Set\[" -e "Type\[" -e "Tuple\[" -e "FrozenSet\[" ./test
grep -nr -e "Dict\[" -e "List\[" -e "Set\[" -e "Type\[" -e "Tuple\[" -e "FrozenSet\[" ./contrib

and then running these and checking if there were anything we missed
grep -nri "from typing import" ./contrib
grep -nri "from typing import" ./test

but only found ones that we couldn't use builtin collection types for

do we have a min supported python version for this project? Only concern is any one running below python 3.9 might not have these builtin collection type hints

Concept ACK 4b9afb1

@theStack
Copy link
Contributor Author

do we have a min supported python version for this project?

Yes, the minimum supported version is documented in doc/dependencies.md:

| [Python](https://www.python.org) (scripts, tests) | [3.9](https://github.com/bitcoin/bitcoin/pull/28211) |

The bump to 3.9 happened a few weeks ago, see #28211.

Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

Approach ACK 4b9afb1

Reducing boilerplate for type hints is nice, and even though there's no deprecation date set, from PEP585 it appears the intent is for the typing types is to be deprecated in favor of the generic types.

From PEP585, it's also recommended to replace Callable and Iterable with their collections.abc alternative, done is this diff:

git diff on 4b9afb1
diff --git a/contrib/seeds/asmap.py b/contrib/seeds/asmap.py
index 84e1811ede..214805b5a5 100644
--- a/contrib/seeds/asmap.py
+++ b/contrib/seeds/asmap.py
@@ -10,9 +10,10 @@ import copy
 import ipaddress
 import random
 import unittest
+from collections.abc import Callable, Iterable
 from enum import Enum
 from functools import total_ordering
-from typing import Callable, Iterable, Optional, Union, overload
+from typing import Optional, Union, overload
 
 def net_to_prefix(net: Union[ipaddress.IPv4Network,ipaddress.IPv6Network]) -> list[bool]:
     """
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index 8ee1ef7008..6665ac4c73 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -20,7 +20,8 @@ import time
 
 from . import coverage
 from .authproxy import AuthServiceProxy, JSONRPCException
-from typing import Callable, Optional
+from collections.abc import Callable
+from typing import Optional
 
 logger = logging.getLogger("TestFramework.utils")
 

Note: from python 3.10 onwards (PEP604), we can replace typing.Union[X, Y] with X | Y - and as a result, typing.Optional[X] with X | None

@fanquake
Copy link
Member

Concept ACK

@theStack
Copy link
Contributor Author

@stickies-v: Thanks, good idea, I've added your suggested diff as a commit. As far as I can see, the only remaining uses of the typing module in the PR branch are now Any, Optional, NoReturn and Union, for which no replacements exist.

Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

ACK a478c81

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK a478c81

@stickies-v
Copy link
Contributor

stickies-v commented Nov 17, 2023

for which no replacements exist.

I think the NoReturn dependency is mostly because of suboptimal main() design, having main() return the exit code would be better imo (regardless of typing dependencies, but can be done later). That would only leave typing.Any (after python 3.10) for which it seems there is no alternative in sight.

git diff on a478c81
diff --git a/test/lint/lint-files.py b/test/lint/lint-files.py
index 86fe727b06..7176a09dff 100755
--- a/test/lint/lint-files.py
+++ b/test/lint/lint-files.py
@@ -11,7 +11,7 @@ import os
 import re
 import sys
 from subprocess import check_output
-from typing import Optional, NoReturn
+from typing import Optional
 
 CMD_TOP_LEVEL = ["git", "rev-parse", "--show-toplevel"]
 CMD_ALL_FILES = ["git", "ls-files", "-z", "--full-name", "--stage"]
@@ -194,7 +194,7 @@ def check_shebang_file_permissions(files_meta) -> int:
     return failed_tests
 
 
-def main() -> NoReturn:
+def main() -> int:
     root_dir = check_output(CMD_TOP_LEVEL).decode("utf8").strip()
     os.chdir(root_dir)
 
@@ -210,10 +210,10 @@ def main() -> NoReturn:
         print(
             f"ERROR: There were {failed_tests} failed tests in the lint-files.py lint test. Please resolve the above errors."
         )
-        sys.exit(1)
-    else:
-        sys.exit(0)
+        return 1
+    
+    return 0
 
 
 if __name__ == "__main__":
-    main()
+    sys.exit(main())

@fanquake fanquake merged commit 98b0acd into bitcoin:master Nov 17, 2023
16 checks passed
@theStack theStack deleted the test-type_hints_use_builtin_collection_types branch November 17, 2023 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants