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

Return type of CogniteResource.__new__ #1342

Closed
anrdnet opened this issue Aug 31, 2023 · 5 comments
Closed

Return type of CogniteResource.__new__ #1342

anrdnet opened this issue Aug 31, 2023 · 5 comments
Assignees

Comments

@anrdnet
Copy link

anrdnet commented Aug 31, 2023

Is your feature request related to a problem? Please describe.
The CogniteResource base class defines

def __new__(cls, *args: Any, **kwargs: Any) -> CogniteResource:

which is a bit odd since it's constructing an instance of cls, not necessarily a CogniteResource directly.

This ambiguity results in different behavior in different tools, which can be seen using the test program

from cognite.client.data_classes.data_modeling import NodeApply

a = NodeApply("foo", "bar", 1)
reveal_type(a)

In mypy there's special handling of __new__, so we get the expected result

note: Revealed type is "cognite.client.data_classes.data_modeling.instances.NodeApply"

but pyright has a more literal interpretation

information: Type of "a" is "CogniteResource"

mypy also has a lot of open issues with handling different return types on __new__, and the behavior has changed several times recently. Relying on these workarounds seems fragile.

Describe the solution you'd like
The signature used in the builtin type stubs for object.__new__ before typing.Self was introduced is akin to

def __new__(cls: type[T], *args: Any, **kwargs: Any) -> T:

This is supported by a developer of pyright.

Describe alternatives you've considered
pyright also has a workaround for

def __new__(cls, *args: Any, **kwargs: Any) -> Any:

since this is often found in other code, but the generic approach seems better since it doesn't rely on workarounds in the different tools.

@haakonvt
Copy link
Contributor

Hi @anrdnet! Our custom __new__ implementation will go away soon in an update to our base classes. You can check out a draft here: https://github.com/cognitedata/cognite-sdk-python/blob/improve-base-cog-classes/cognite/client/data_classes/_base.py

Any thoughts and proposals are very welcome 😄

@anrdnet
Copy link
Author

anrdnet commented Aug 31, 2023

Hi @haakonvt, that looks great! I'll keep an eye on #1151, any thoughts on when can we expect it to be released?

@haakonvt
Copy link
Contributor

It grew into a monstrosity, so I'm currently breaking it up into smaller PRs. Hopefully 1-2 weeks time 😄

@haakonvt
Copy link
Contributor

@anrdnet Hey again! As of 6.25.1 this has been implemented (dunder methods __new__ and __getattribute__ are gone). Could you check it out and report back if it is working as expected? If there are more/better ways to do this, let me know 😄

@anrdnet
Copy link
Author

anrdnet commented Sep 18, 2023

Pyright seems happy with the types now, thanks!

@anrdnet anrdnet closed this as completed Sep 18, 2023
@haakonvt haakonvt self-assigned this Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants