youtux / types-factory-boy

Type stubs for factory-boy
MIT License
18 stars 4 forks source link

Fix no build attribute issue #15

Closed jimmylai closed 1 year ago

jimmylai commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

jimmylai commented 1 year ago

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 commented 1 year ago

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

jimmylai commented 1 year ago
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?

jimmylai commented 1 year ago

@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 commented 1 year ago

Hi, I just released v0.4.0