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

Replace eve.concepts.AnyNode by eve.concepts.BaseNode #574

Closed
wants to merge 2 commits into from
Closed

Replace eve.concepts.AnyNode by eve.concepts.BaseNode #574

wants to merge 2 commits into from

Conversation

jdahm
Copy link
Contributor

@jdahm jdahm commented Dec 4, 2021

Description

This removes the TypeVar from concepts, which means these can be used in ClassVar instances.

Resolves #565.

@jdahm jdahm requested review from havogt and egparedes and removed request for havogt December 4, 2021 00:40
@jdahm jdahm changed the title Replace AnyNode by BaseNode Replace eve.concepts.AnyNode by eve.concepts.BaseNode Dec 4, 2021
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

The tests should be fixed. Other than that it looks fine, although I would like to discuss if AnyNode should be removed as you propose or just renamed to NodeT

@@ -116,11 +115,10 @@ class Config:
_EVE_NODE_INTERNAL_SUFFIX = "__"
_EVE_NODE_IMPL_SUFFIX = "_"

AnyNode = TypeVar("AnyNode", bound="BaseNode")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should delete the AnyNode TypeVar definition, since it has a different meaning than BaseNode. Example:

def f1(a: BaseNode) -> BaseNode:
    ...

def f2(a: AnyNode) -> AnyNode:
    ...

f2 always returns the same BaseNode subclass it got as an argument, while f1 may return a different subclass. Maybe it could be renamed to NodeT to make clear that is a TypeVar.

@jdahm
Copy link
Contributor Author

jdahm commented Dec 6, 2021

@egparedes Apologies, I meant to convert this to a draft before I left for the day on Friday. There many cascading type issues that mypy catches that need to be solved first. Your comment might help with that.

@jdahm
Copy link
Contributor Author

jdahm commented Aug 2, 2022

Closing this for inactivity. Will re-examine when I have time.

@jdahm jdahm closed this Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClassVar parameters cannot include any type variables
2 participants