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

Enable type checking for users and add types to nearly everything #156

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dhdaines
Copy link
Contributor

@dhdaines dhdaines commented Jul 6, 2024

Some of the code (pipeline.py) had some types. But this wasn't super useful because the magical py.typed file was not included in the package.

So this adds that, but also adds some types.

I can't totally figure out how to use Protocol correctly for pipeline functions but in actual fact they have a fixed signature, and the attributes we add are purely for internal use, so it's type: ignore for the time being.

@dhdaines dhdaines mentioned this pull request Jul 6, 2024
@dhdaines
Copy link
Contributor Author

dhdaines commented Jul 6, 2024

fixes #158

@dhdaines dhdaines changed the title Enable type checking for users and add types to basic API Enable type checking for users and add types to nearly everything Jul 6, 2024
@dhdaines
Copy link
Contributor Author

dhdaines commented Jul 6, 2024

I got carried away and added types nearly everywhere.

For CompleteSet there is an issue since its API is quite strange (though I understand the intent)

For Vector this can't be done efficiently because it relies on mixing str and float everywhere including in the values, so numeric calculations are simply undefined for string vectors (otherwise we'd have to cal isinstance repeatedly, which is super slow)

@dhdaines
Copy link
Contributor Author

dhdaines commented Jul 6, 2024

Final comment: any good reason why boost needs to be an int? It's a number in lunr.js and it just gets multiplied into a float anyway at some point.

lunr/utils.py Show resolved Hide resolved
lunr/query.py Show resolved Hide resolved
Copy link
Owner

@yeraydiazdiaz yeraydiazdiaz left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to do this, I've been meaning to add types for a long time.

The PR will likely need reformatting and I suggest squashing the commits as well, but thanks for separating things so it's easier to review.

@@ -64,5 +67,7 @@ def add(self, term, field, metadata):
else:
self.metadata[term][field][key] = metadata[key]

def __eq__(self, other):
def __eq__(self, other: object):
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this not also MatchData as above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because __eq__ by definition has to accept any object as an argument, even if a comparison with that object is not possible - mypy will give a specific warning about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like this should be documented in mypy's documentation somewhere but I can't figure out where! It's also explained here: https://stackoverflow.com/questions/54801832/mypy-eq-incompatible-with-supertype-object

lunr/query_lexer.py Show resolved Hide resolved
@dhdaines
Copy link
Contributor Author

dhdaines commented Sep 9, 2024

Formatted, type-checked, toxed, squashed and force-pushed!

@dhdaines
Copy link
Contributor Author

dhdaines commented Sep 9, 2024

The PR will likely need reformatting and I suggest squashing the commits as well, but thanks for separating things so it's easier to review.

Question - should I rebase the other PRs on top of this one? That will take some editing because of all of the types.

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2024

Codecov Report

Attention: Patch coverage is 95.60000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 95.82%. Comparing base (d07b60f) to head (5a2f171).

Files with missing lines Patch % Lines
lunr/index.py 82.97% 8 Missing ⚠️
lunr/match_data.py 88.88% 1 Missing ⚠️
lunr/query_parser.py 95.83% 1 Missing ⚠️
lunr/token_set.py 95.83% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
- Coverage   96.02%   95.82%   -0.21%     
==========================================
  Files          48       48              
  Lines        3171     3257      +86     
==========================================
+ Hits         3045     3121      +76     
- Misses        126      136      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

3 participants