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

Add missing generic on Factory subclasses #1060

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Feb 6, 2024

Following #1059, subclasses were missing the type var, making it impossible to parametrize at runtime.


There's a couple places where I'd like to see typing added (e.g. LazyAttribute and friends' callables arguments). They should be pretty straightforward, would you like to have a PR implementing it?

Copy link
Member

@rbarrois rbarrois left a comment

Choose a reason for hiding this comment

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

Thanks for catching that!

That's a good point for the abstract factories; for the concrete ones (StubFactory, ListFactory, DictFactory), I believe we should use the actual class instead of the typevar'd.

Finally, would you be able to add a couple of tests in tests/test_typing.py? Those are exercised through make test as the first check.

Thanks!

factory/base.py Outdated Show resolved Hide resolved
factory/base.py Outdated Show resolved Hide resolved
factory/base.py Outdated Show resolved Hide resolved
@Viicos
Copy link
Contributor Author

Viicos commented Feb 7, 2024

Good catch on concrete factories. I updated tests to make use of assert_type, which is more convenient when dealing with type tests

@rbarrois
Copy link
Member

rbarrois commented Feb 7, 2024

I add tried to use assert_type as well, however it is only available from Python 3.11 — hence the typing tests with mypy :)

@Viicos
Copy link
Contributor Author

Viicos commented Feb 7, 2024

Yes this is why I added typing_extensions as a tox dependency

@Viicos
Copy link
Contributor Author

Viicos commented Mar 28, 2024

Hi @rbarrois, any chance we could get a release once this is merged? The typing addition from the previous PR would be really useful in test code as it really plays well in IDEs :) Thanks!

from sqlalchemy.exc import IntegrityError
from sqlalchemy.orm.exc import NoResultFound

from . import base, errors

T = TypeVar("T")
Copy link
Contributor

@foarsitter foarsitter Sep 10, 2024

Choose a reason for hiding this comment

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

Is it required to create a new TypeVar here? Can't we import it from base.py or user base.T? The same applies todjango.py and mogo.py.

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.

3 participants