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

Fix no build attribute issue #15

Merged
merged 4 commits into from
Feb 28, 2023
Merged

Fix no build attribute issue #15

merged 4 commits into from
Feb 28, 2023

Conversation

jimmylai
Copy link
Contributor

Given this example code:

from factory import Factory

class MyFactory(Factory):
    ...

MyFactory().build()

Mypy complains

a.py:6: error: "NoReturn" has no attribute "build"  [attr-defined]
Found 1 error in 1 file (checked 1 source file)

It's caused by the wrong NoReturn type annotation.

The return value of new() should be the new object instance (usually an instance of cls).
https://docs.python.org/3/reference/datamodel.html#object.__new__

With this fix, mypy is happy:

Success: no issues found in 1 source file

@youtux
Copy link
Owner

youtux commented Feb 27, 2023

can you show a full executable example?
I don't quite understand why you would ask the type of MyFactory().build(), since MyFactory() would give you the model instance already, so what do you expect .build() to return?

@youtux
Copy link
Owner

youtux commented Feb 27, 2023

Anyway, you are right about the return type of __new__, but I think we should better annotate it as typing.Self

@jimmylai
Copy link
Contributor Author

The question makes sense. However, this code pattern is used hundreds times in our private codebase and this type error becomes noises.
I'll change to use typing.Self which is only available in 3.11. To maintain backward compatibility to old Python version, I'll add an if condition on the import and import it from type_extensions when needed.

@jimmylai
Copy link
Contributor Author

It looks like the existing imports only import from typing_extensions. I'll simply follow the same practice then.

@jimmylai
Copy link
Contributor Author

py38: commands[0]> mypy .
src/factory-stubs/builder.pyi:27: error: A function returning TypeVar should receive at least one argument containing the same TypeVar  [type-var]
        def copy(self) -> TDeclarationSet: ...
                          ^
src/factory-stubs/builder.pyi:27: note: Consider using the upper bound "DeclarationSet" instead
src/factory-stubs/django.pyi:51: error: A function returning TypeVar should receive at least one argument containing the same TypeVar  [type-var]
        def copy(self) -> mute_signals_T: ...
                          ^
src/factory-stubs/django.pyi:51: note: Consider using the upper bound "mute_signals" instead
Found 2 errors in 2 files (checked 15 source files)

There are some TypeVar related errors defined and can be replaced by Self now.
Just fixed them with the new commits.

@youtux can you approve the workflow run to verify the new changes?

@youtux youtux merged commit 55a4a1e into youtux:main Feb 28, 2023
@jimmylai
Copy link
Contributor Author

@youtux thanks for merging my PR.
Do you plan to make a release soon? We're excited to apply the fix. Let me know if there is anything I can help.

@youtux
Copy link
Owner

youtux commented Feb 28, 2023

Hi, I just released v0.4.0

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.

2 participants