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 typing support to get_solo #128

Merged
merged 10 commits into from
Jan 15, 2024
Merged

Add typing support to get_solo #128

merged 10 commits into from
Jan 15, 2024

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Oct 12, 2023

As get_solo hits the cache, type checkers can't infer the return type of the method correctly. User could use typing.cast or similar but having it fixed in the library directly sounds easier. typing.TypeVar is used as a workaround until typing.Self can be used without making use of typing_extensions.

As `get_solo` hits the cache, type checkers can't infer the return type of the method correctly.
User could use `typing.cast` or similar but having fixed it in the library directly sounds easier.
`typing.TypeVar` is used as a workaround until `typing.Self` can be used without making use of `typing_extensions`.
@Viicos
Copy link
Contributor Author

Viicos commented Oct 12, 2023

If you'd like to, we could add basic typing to the other parts of the library as well

@johnthagen
Copy link
Collaborator

johnthagen commented Jan 13, 2024

I could see value in adding typing to this library, and enabling mypy checking of the project as a new CI check.

An example you could look at is:

Please configure Mypy in strict mode in pyproject.toml like:

[tool.mypy]
ignore_missing_imports = true
strict = true

We'll also want to add a py.typed file to the solo package and include it in MANIFEST.in.

@Viicos
Copy link
Contributor Author

Viicos commented Jan 14, 2024

Great to hear you want to head into this direction. I added types to the whole library, and also updated some old compatibility code. Let me know if any changes are needed. As another PR, I think it would be great to have:

  • black + isort/ruff
  • Use PEP 517/518 for packaging

I you agree with this, I can make separate PRs

solo/models.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@johnthagen
Copy link
Collaborator

Great to hear you want to head into this direction. I added types to the whole library, and also updated some old compatibility code. Let me know if any changes are needed. As another PR, I think it would be great to have:

  • black + isort/ruff
  • Use PEP 517/518 for packaging

I you agree with this, I can make separate PRs

Let's make separate PRs for these things.

I'd prefer using Ruff for everything on the first bullet: formatting, linting and isort rules. You can see my example repo here for how I usually configure Ruff:

We may need to modify some of these rules for django-solo (e.g. we should keep the first diff a bit minimal rather than enabling too many of those rules).

solo/models.py Show resolved Hide resolved
solo/models.py Show resolved Hide resolved
@johnthagen
Copy link
Collaborator

johnthagen commented Jan 14, 2024

I presume you are using django-solo in a production application. Could you test your application with this PR to smoke test that everything here works well for you at runtime?

Would also be great if you could double check that Mypy now type checks your usage of django-solo properly when installed from this PR.

@Viicos
Copy link
Contributor Author

Viicos commented Jan 14, 2024

I presume you are using django-solo in a production application. Could you test your application with this PR to smoke test that everything here works well for you at runtime?

We are making use of django-solo extensively on an open source project, so I can try it there, I'll let you know tomorrow how it goes

@Viicos
Copy link
Contributor Author

Viicos commented Jan 15, 2024

Here is a pull request making use of my branch:

I had to add back get_cache as we were making use of it in a 3rd party library (I've also added a deprecation warning).

Regarding mypy, we don't run it yet, but I can already see from my IDE (which uses pyright) that .get_solo is correctly typed. Apart from that, tests are passing

@johnthagen
Copy link
Collaborator

Could you add a changelog entry for deprecating get_cache?

@Viicos
Copy link
Contributor Author

Viicos commented Jan 15, 2024

Could you add a changelog entry for deprecating get_cache?

See c279dc4

@Viicos
Copy link
Contributor Author

Viicos commented Jan 15, 2024

Looking at the remaining type issue, might be another mypy bug..

@Viicos
Copy link
Contributor Author

Viicos commented Jan 15, 2024

This is a mypy bug (works with pyright), but I can't produce a MRE: playground. Maybe this only happens when split into multiple files

@johnthagen
Copy link
Collaborator

Thanks for the contribution, @Viicos!

@johnthagen johnthagen merged commit 20d5ce6 into lazybird:master Jan 15, 2024
5 checks passed
@Viicos Viicos deleted the patch-1 branch January 15, 2024 15:15
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