search

carltongibson / django-filter

A generic system for filtering Django QuerySets based on user selections
https://django-filter.readthedocs.io/en/main/
Other
4.41k stars 766 forks source link

Add types for DjangoFilterBackend #1585

Closed last-partizan closed 1 year ago

last-partizan commented 1 year ago

After recent changes to pyright, we need to add proper types for django-filters for it to pass type-checking.

Required changes is only filter_queryset types, but i have added other as well.

This is draft, and it still not working as expected, but i will update this and djangoresframework-types after some discussion in pyright repo, on how to do this better.

Main problem is that djangorestframework-types have included MongoQuerySet in it's types, and i don't want to drag it here. Maybe it's better to replace it with some sort protocol there.

last-partizan commented 1 year ago

Update: we removed MongoQuerySet support from djangorestframework types and now this is ready to merge. It's working as expected with latest pyright.

@carltongibson can we merge this? or maybe you have some suggestions for improvements?

carltongibson commented 1 year ago

Hi. TBH I'm not keen on the inline annotations here. It seems like a lot of noise. 🤔

Could you use a stub file?

(Didn't I read they changed the Optional syntax?)

last-partizan commented 1 year ago

Yes, new Optional and Union syntax available starting from Python 3.10, but probably we can use it with from __future__ import.

I've cleaned up it a bit and updated to use new syntax.

I theory, yes, i can use type stub - but it would require me to maintain separate package which will break if anything changed here.

It may seem as noise at first, and typing in python has some rough edges, but developer expirience is improved when we have types. I'm started adding types to my projects a few years ago, and it helped me to catch a few bugs.

That's especially useful when types are added to projects, not as separate stub files.

carltongibson commented 1 year ago

Nothing is going to change here 😅

last-partizan commented 1 year ago

Oh, okay.

last-partizan commented 1 year ago

@carltongibson btw, are you completely opposed to having types in the project, or just don't like the idea of types being in the same file?

I can refactor this to use separate .pyi, so it won't pollute code file, but types still be accessible.

carltongibson commented 1 year ago

Hi @last-partizan -- good hustle 😜

I'm not totally opposed to the idea of the stub file...

My concerns would be:

  1. I'd have to maintain it.
  2. I don't really see the point of types without type checking, so there's (massive) scope creep here just getting out the gate.

I'd lean towards you just adding a stub file in your own project to begin, but if you want to open an exploratory PR here, I'm happy to look at that (and I would do so with an open mind.)

Hope that makes sense.

last-partizan commented 1 year ago

Yeah, maintaining separate type files is a fuss, that's why i like types in a code.

And maintaining my own stub files is yet bigger fuss.

I'd probably just use my fork with types, it would be easier to maintain in case of changes.

Have a nice day!