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(types): Add type hints and code optimizations #248

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

benatouba
Copy link
Contributor

DISCLAIMER: I know this is a lot of changes and might be rejected or a lot of changes requested. But I like to work with typed functions much better and think someone might also benefit from this. This will definitely require additional changes in the future, but I feel it could be a start. All tests passed locally (no google API key).

Python3.8 is almost end-of-life and backwards compatibility to versions before type hints is not a priority any more.

This commit is a start and by no means complete or correct for all instances.

Furthermore, I included code optimizations. These mainly regard raising of exceptions, pytest style tests and some minor refactoring.

  • tests added / passed
  • passes git diff upstream/master | flake8 --diff
    This is not true for some lines, but most of these would not pass a default flake8 config without the changes.
    Formating is done via ruff formatter in black style.
  • Introduces Type annotations/type hinting for easier use of the library

@fmaussion
Copy link
Owner

Thanks! I'm away on leave starting tomorrow so I wont have a look until later in the summer, but a few things already:

  • this is too much for one commit - it needs to be split into separate commits

  • I'm okay with black, but this has to be done in a separate commit only and this needs to be removed from blame. We also need to enforce this in CI then

  • I'm also okay with type hints although I always thought its overkill for a library like salem

  • but somehow there are still some salem users around. Maybe I should do a survey one day.

Python3.8 is almost end-of-life and backwards compatibility
to versions before type hints is not a priority anymore.

This commit is a start and by no means complete or correct
for all instances.

Furthermore I included code optimizations. These mainly regard
raising of exceptions, pytest style tests and some minor
refactoring.
@benatouba
Copy link
Contributor Author

Ok. I will split this up into multiple, well organized commits. However, this will only happen in a couple of weeks.

  • I actually do not really like black's formatting style, but was unsure of the style guide used in the project.
  • Thank you for the tip to remove from blame. I did not know that this was possible.

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