-
-
Notifications
You must be signed in to change notification settings - Fork 172
refactor: migration to Python 3.10 #2722
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
base: main
Are you sure you want to change the base?
Conversation
|
As I see, there still a lot of issues, so I'll complete refactor |
|
@fubuloubu, could you verify PR? |
fubuloubu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fantastic! Thanks for taking the time and submit this. Left a few small comments
| Returns: | ||
| Optional[:class:`~ape.api.networks.ProxyInfoAPI`]: Returns ``None`` if the contract | ||
| ProxyInfoAPI | None: Returns ``None`` if the contract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still show up the same if we avoid the class syntax?
| "ContractMethodQuery", | ||
| ] | ||
| QueryType = ( | ||
| "BlockQuery | BlockTransactionQuery | AccountTransactionQuery | ContractCreationQuery | ContractEventQuery | ContractMethodQuery" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably are able to split this up to make it a little nicer
| "BlockQuery | BlockTransactionQuery | AccountTransactionQuery | ContractCreationQuery | ContractEventQuery | ContractMethodQuery" | |
| "BlockQuery" | |
| " | BlockTransactionQuery" | |
| " | AccountTransactionQuery" | |
| " | ContractCreationQuery" | |
| " | ContractEventQuery" | |
| " | ContractMethodQuery" |
| extra_addresses (list | None): Additional contract addresses containing the same event type. Defaults to | ||
| Additional contract addresses containing the same event type. Defaults to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| extra_addresses (list | None): Additional contract addresses containing the same event type. Defaults to | |
| Additional contract addresses containing the same event type. Defaults to | |
| extra_addresses (list | None): Additional contract addresses containing the same event type. Defaults to |
| from typing import get_args, get_origin, Union | ||
| from types import UnionType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can put these imports at the top no problem
| def convert_method_kwargs(self, kwargs) -> dict: | ||
| fields = TransactionAPI.__pydantic_fields__ | ||
|
|
||
| def get_real_type(type_): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To what degree were you able to test this working with the new changes?
Wait. Since checking failing tests, I feel we need to accomplish a refactor first #2724 |
What I did
fixes:
Fixes #2698
Migrated type hints to Python 3.10+ syntax:
Optional[T]withT | NoneUnion[A, B, ...]withA | B | ...How I did it
OptionalandUnionwith|syntax throughout the codebaseconverters.pyto handle both old (Union/Optional) and new (|) syntax usingget_origin()andget_args()pyproject.tomlandsetup.pyto require Python 3.10+ignore_cleanup_errors=TruetoTemporaryDirectorycallsHow to verify it
Run the test suite:
To be clear, there were already failing tests in the baseline. My changes didn't introduce any regressions (and actually fixed 1 test).
Checklist