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

feat: Add basic typing support #903

Closed
wants to merge 12 commits into from

Conversation

last-partizan
Copy link
Contributor

This PR adds basic type hinting support

Refs #468

Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

That's interesting, thanks!

I've got a few questions though:

  • How could we ensure that the annotations are actually working and useful? Can we test those?
  • There doesn't seem to be a simple way to link the Generic[T] with the class Meta: model

@last-partizan
Copy link
Contributor Author

@rbarrois

hmm, i think, to properly test type annotations, we should add pyright and/or mypy to CI, run it over example file, and compare that to expected output.

But there is simpler option, we can do something like this:

def x() -> int:
    ...

assert x.__annotations__ == {"returns": int}

In theory, this should be enough to ensure we get expected annotations.

as for linking with Meta: model, yes, i don't see any way to do it.

Also, i see some failing CI checks, i'll try to fix it.

@last-partizan
Copy link
Contributor Author

Oh, looks like all errors is from 3.6 version, lefetime of which is ended three weeks ago.

Maybe better solution in this case, is to remove old version from CI? But i'll try to find other solutions, maybe any ideas?

@last-partizan
Copy link
Contributor Author

I haven't found easy workarounds for this, so i created PR for removing 3.6 from CI, and adding 3.10 and django-4.0.

@mathieutu
Copy link

Thanks, an other huge step would be to type the kwargs.
To be able to do UserFactory.create(xxx=yy) and have autocompletion on xxx.

I have no idea on how to do that, and even if it could be possible with python? 🤔

@last-partizan
Copy link
Contributor Author

I think, it could be implemented. Python 3.10 added ParamSpec, and it could be used with older versions of python via typing-extensions, so i think it could be possible.

@last-partizan
Copy link
Contributor Author

I found a way to properly type even calls to factory, turned out it was pretty simple.

Now types are correct with both possible ways to call a factory:

BookFactory() -> Book
BookFactory.create() -> Book

But, this all requires python 3.6 at least.

@mathieutu
Copy link

I think, it could be implemented. Python 3.10 added ParamSpec, and it could be used with older versions of python via typing-extensions, so i think it could be possible.

I would love to see that!

@last-partizan
Copy link
Contributor Author

I found a way to properly type even calls to factory, turned out it was pretty simple.

Turned out it doesn't work. Reverted changes back to only typing create, build, etc...

Now i'm trying to test it.

@last-partizan
Copy link
Contributor Author

As for testing type annotations, i found one way to do it:

And it looks like this: https://github.com/smarie/python-decopatch/pull/27/files

  • Install pyright
  • Run it in json mode, compare results to snapshot

I could add similar tests for this package. (In this PR or next, preferrably next)

@last-partizan
Copy link
Contributor Author

I fund myself today thinking it would be great to have types in my factories. Can we merge this, or we need to add tests first?

@youtux
Copy link

youtux commented Jun 3, 2022

I started a project, types-factory-boy, to add type annotations to factory boy. It is in early stage, and it most probably needs a plugin to automatically discover and create type annotations automatically (otherwise you would have to type annotate basically every attribute), but I think it's a start.

@flaeppe
Copy link

flaeppe commented Jul 14, 2022

When it comes to testing types and type annotations. I'd suggest: https://github.com/TypedDjango/pytest-mypy-plugins

Although it's a pytest plugin, so those tests needs to be run with pytest. Should be possible to add that as an individual job in CI etc.

Have a look at django-stubs https://github.com/typeddjango/django-stubs/tree/master/tests/typecheck for reference of how to use it and/or set it up.

@last-partizan
Copy link
Contributor Author

@youtux great project, thank you.

After pyright fixed a bug with invalid init annotations and default values in 1.1.264, default factory_boy is throwing type errors sometimes (Maybe declaration for example).

Your project fixed it.

@jeffwidman
Copy link
Member

Is this ready for review again?

@last-partizan
Copy link
Contributor Author

In basic form, yes.

But, we have https://github.com/youtux/types-factory-boy now, and i can try to merge it into this project with merge_pyi.

But, i think it's better to leave this PR in minimal form, maybe add tests for types in the next PR. And then - merge types-factory-boy for more complete type coverage.

@youtux
Copy link

youtux commented Nov 5, 2022

I think there was some misunderstanding here, as I don't understand @last-partizan message about merging types-factory-boy into this codebase.

types-factory-boy contains annotations for factory_boy lib (which I don't control), not for pytest_factoryboy (that I manage).

EDIT: scrap that, I thought I was writing in the pytest_factoryboy project.

@youtux
Copy link

youtux commented Nov 6, 2022

I made some progress in types-factory-boy. Now there is a mypy plugin that automatically adds the typevar information of the Factory[T] baseclass.

So, instead of writing this:

class AuthorFactory(Factory[Author]):
    class Meta:
        model = Author
reveal_type(AuthorFactory.create())  # foo.Author

we can write this:

class AuthorFactory(Factory):  # No need to specify the typevar for Factory[...], it's inferred from Meta.model
    class Meta:
        model = Author
reveal_type(AuthorFactory.create())  # foo.Author

@palgorhythm
Copy link

Any updates on whether this PR will be merged?

@Viicos
Copy link
Contributor

Viicos commented Dec 3, 2023

Gentle bump @francoisfreitag considering how many people requested it

@francoisfreitag
Copy link
Member

I don’t see opposition to merging this work. I offered a small fixes as a PR. Once these fixes are correct, and integrated into this PR, I think we can merge it.

In the meantime, @rbarrois’ opinion is very welcome :)

@last-partizan
Copy link
Contributor Author

Oh, I missed that PR. Fixes accepted!

Copy link
Member

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

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

Looks like a good start. Let’s hope type information PR build up to better document the library :)

I’ll leave the PR sit for a couple more weeks, to give time for others to report their concerns. In the absence of opposition, I’ll merge it early 2024.

Copy link
Contributor

@Viicos Viicos left a comment

Choose a reason for hiding this comment

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

I think it doesn't hurt to type size as well

factory/base.py Outdated Show resolved Hide resolved
factory/base.py Outdated Show resolved Hide resolved
@francoisfreitag
Copy link
Member

Can you rebase one last time before merging, please? 🙏

@last-partizan
Copy link
Contributor Author

Sure, rebased already, but looks like typing tests aren't running in CI.

Almost ready.

@last-partizan
Copy link
Contributor Author

Got it!

Ready to be merged, but please take another look at github workflow file and tox.ini

tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
last-partizan and others added 2 commits January 18, 2024 12:36
Co-authored-by: François Freitag <mail@franek.fr>
Co-authored-by: François Freitag <mail@franek.fr>
@rbarrois
Copy link
Member

Thanks for all the hard work! (And sorry for my long absence :/)

I'm going to do a final pass, before merging this; I'm likely going to rebase or squash some of the commits, for easier further maintenance!

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 a lot for the work on this!

The basic typing annotation look fine, and are a great starting point; I'd like to merge them as quickly as possible.

I'm not too sure about the py.typed file though: it's unclear to me whether it means that the project is fully typed, or merely that it supports some typing annotations — PEP 561 is not very clear on that.

Regarding the testing setup, I have met a few issues running it locally; for now, I would rather merge the annotated code without those tests, or with some much simpler ones (e.g using mypy.reveal_types directly).

Suggested next steps:
@last-partizan if you're OK with this, I propose rebasing your branch to squash the various intermediate commits, and simplifying the test setup to reduce the amount of extra deps.
Would that sound right with you?

Details on the testing issues below:

  • The make test-typing only works in a blank virtualenv without, e.g., django being installed. This is not the way we run tests usually: we use make test and others in a venv where make update was run to setup all dependencies.
  • I would rather limit the testing dependencies to the stdlib if possible; depending on pytest would also confuse new contributors as to the way to run the test suite.

@@ -59,6 +59,10 @@ doc =
sphinx_rtd_theme
sphinxcontrib-spelling

[options.package_data]
factory =
Copy link
Member

Choose a reason for hiding this comment

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

It would help to explain why this file is here, as it's not an actual "data" file.

Suggested change
factory =
# Mark the module as typed, see PEP 561
factory =

Copy link
Member

Choose a reason for hiding this comment

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

Based on PEP 561, I'm not sure we can add this yet: the project is not fully typed.

@last-partizan
Copy link
Contributor Author

@rbarrois i'm ok with whatever fits you.

If you want to simplify tests or remove py.typed - go ahead.

My main interest - is getting started with typing this package, and then improve types when i encounter problems in my projects.

@Viicos
Copy link
Contributor

Viicos commented Jan 18, 2024

Regarding the testing setup, I have met a few issues running it locally; for now, I would rather merge the annotated code without those tests, or with some much simpler ones (e.g using mypy.reveal_types directly).

One alternative to pytest-mypy-plugins is to do it "manually": you create a python file that you want to test with mypy (or any type checker), and use typing_extensions.assert_type. You can assert for errors as well, by adding a type: ignore comment and making sure that mypy is configured to flag unused type: ignore comments as an error.

This solution does require some more work, but is imo more robust as you don't rely on how mypy errors messages are formatted (that could change from one version to another), and can be ported to another type checker if necessary (assert_type is standard Python).

Here is an example of how I do it (test files are under testfiles, and test_stubs.py is where the actual testing happens).

Based on PEP 561, I'm not sure we can add this yet: the project is not fully typed.

I'm not sure it should be included either. Without it, IDEs should be smart enough to still use the type definitions, but type checking with the command line will not.

@rbarrois
Copy link
Member

Thanks again for all this work, I have merged it as #1059 !

@rbarrois rbarrois closed this Jan 18, 2024
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.

None yet