-
-
Notifications
You must be signed in to change notification settings - Fork 457
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 Django 4.0 trigram word classes #1278
Add Django 4.0 trigram word classes #1278
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR
@@ -104,9 +104,27 @@ class SearchHeadline(Func): | |||
class TrigramBase(Func): | |||
def __init__(self, expression: _Expression, string: str, **extra: Any) -> None: ... | |||
|
|||
class TrigramWordBase(Func): | |||
def __init__(self, expression: _Expression, string: str, **extra: Any) -> None: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm Django documentation has the arguments in the other order, for TrigramWordSimilarity
https://docs.djangoproject.com/en/4.1/ref/contrib/postgres/search/#django.contrib.postgres.search.TrigramWordSimilarity
TrigramWordSimilarity(string, expression, **extra)
Or is the documentation wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation is correct and I need to fix this, thanks for double checking!
IMO it's super confusing that the arguments are swapped with the other trigram functions, but the rationale is given in the Django PR: django/django#14833 (comment)
function: str | ||
|
||
class TrigramStrictWordSimilarity(TrigramWordBase): | ||
function: str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Func
base class already defines function: str
, no need to repeat attributes already defined higher in class hierarchy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can do the same thing for arg_joiner
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
CI will be fixed in #1279 |
e756d39
to
e89e573
Compare
Thanks both of you! |
I have made things!
Documentation for these classes:
https://docs.djangoproject.com/en/4.1/ref/contrib/postgres/search/#trigramwordsimilarity
Related issues