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

Move type annotations inline #290

Merged
merged 3 commits into from
Apr 27, 2024
Merged

Move type annotations inline #290

merged 3 commits into from
Apr 27, 2024

Conversation

lovetox
Copy link
Contributor

@lovetox lovetox commented Apr 22, 2024

This moves all type annotations inline

  • I did not touch any code logic
  • I had to place 4, 5 type: ignore on some lines because pyright thinks there are errors, this should be checked after this is merged
  • I added a pyright config to the pyproject.toml
  • pyright in strict mode checks without errors
  • all other folders are ignored for now (docs, util, test, examples ... )

This does not mean you need to use pyright as a type checker for this project.
The config section simply allows developers who use pyright to pick up the config.

@cvzi
Copy link
Contributor

cvzi commented Apr 23, 2024

I need a few days before I have time to look at this in detail.

Can you enable and run the github action on your forked repository to test all Python versions? You should be able to run it manually for each branch.

It fails on older Python on my machine.

It there is no easy fix, maybe we could drop support for end-of-life Python versions. We would need to check how often the package is still used on those versions at the moment (ref #243 )

emoji/core.py Outdated Show resolved Hide resolved
@lovetox
Copy link
Contributor Author

lovetox commented Apr 23, 2024

hi, all checks run now trough on all python versions

But this is only possible with a new dependency

  • typing_extensions which provides type annotations which are backwards compatible

If we don't want this dependency then our options are

  • Losing the Literal type, this would not be a big problem, as we can still type the argument as str
  • Lose the TypedDict and replace it with a dict[str, Any]

emoji/core.py Outdated Show resolved Hide resolved
emoji/core.py Outdated Show resolved Hide resolved
@lovetox
Copy link
Contributor Author

lovetox commented Apr 25, 2024

We could also use the Match class from typing_extensions this will not break with 3.13, depending on the decision if you want to use typing_extensions

@lovetox
Copy link
Contributor Author

lovetox commented Apr 27, 2024

PR rebased, and code review suggestions squashed

@lovetox
Copy link
Contributor Author

lovetox commented Apr 27, 2024

I added some CI updates

  • Upgraded the actions
  • Added a new "lint" job, where we can gather different linting steps in the future
  • Its not necessary to execute linting with a python matrix, because the most linters can be configured to test with a specific version in mind (e.g. see pyproject.toml -> tool.pyright)

@cvzi
Copy link
Contributor

cvzi commented Apr 27, 2024

Looks good to me. The typing_extensions dependency is ok with me.

@TahirJalilov I think you can merge this too, but let's wait with a new release.

I will look into solving some of these type: ignore. And also while I reviewed this PR, I found some unrelated problems with the tests/pytest that I want to solve.

@lovetox
Copy link
Contributor Author

lovetox commented Apr 27, 2024

As the typing extension is accepted, i fixed the last thing that we now use the Match object from typing_extensions which should not break with 3.13

@lovetox lovetox requested a review from cvzi April 27, 2024 10:32
@TahirJalilov TahirJalilov merged commit cf2494c into carpedm20:master Apr 27, 2024
8 checks passed
@TahirJalilov
Copy link
Collaborator

Looks good to me. The typing_extensions dependency is ok with me.

@TahirJalilov I think you can merge this too, but let's wait with a new release.

I will look into solving some of these type: ignore. And also while I reviewed this PR, I found some unrelated problems with the tests/pytest that I want to solve.

ok @cvzi Thank you!

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

3 participants