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 Django 4.0 trigram word classes #1278

Merged
merged 2 commits into from Dec 5, 2022

Conversation

michael-lazar
Copy link
Contributor

I have made things!

Documentation for these classes:

https://docs.djangoproject.com/en/4.1/ref/contrib/postgres/search/#trigramwordsimilarity

Related issues

Copy link
Collaborator

@intgr intgr 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 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: ...
Copy link
Collaborator

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?

Copy link
Contributor Author

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
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

@intgr intgr left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@intgr
Copy link
Collaborator

intgr commented Dec 3, 2022

CI will be fixed in #1279

@sobolevn sobolevn merged commit 3210e2d into typeddjango:master Dec 5, 2022
@sobolevn
Copy link
Member

sobolevn commented Dec 5, 2022

Thanks both of you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Missing TrigramWordSimilarity, TrigramWordDistance
3 participants