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

SearchFilter.get_search_terms returns list. #9338

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Mar 22, 2024

#9017 unintentionally treated SearchFilter.get_search_terms as a private method, changing its signature from a list of strings, to a string.

This pull request reverts that change, by pulling the "search_smart_split" inside the method, ensuring that search terms are correctly tokenised by "quoted strings like this" and comma, sperated, strings, like, this.

Closes #9308

Copy link
Contributor

@jthevos jthevos left a comment

Choose a reason for hiding this comment

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

🚢 🚢 🚢

@tomchristie tomchristie merged commit eb361d2 into master Mar 22, 2024
9 checks passed
@tomchristie tomchristie deleted the get-search-terms-returns-list branch March 22, 2024 10:52
@tomchristie tomchristie mentioned this pull request Mar 22, 2024
@@ -24,15 +24,19 @@ def search_smart_split(search_terms):
"""generator that first splits string by spaces, leaving quoted phrases together,
Copy link

Choose a reason for hiding this comment

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

Came from your haiku
Just to add 🐼 comment
Docstring needs fix

(“generator” -> Create list)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll put up a PR for it!

@@ -85,7 +89,8 @@ def get_search_terms(self, request):
"""
value = request.query_params.get(self.search_param, '')
field = CharField(trim_whitespace=False, allow_blank=True)
return field.run_validation(value)
cleaned_value = field.run_validation(value)
return search_smart_split(cleaned_value)
Copy link
Contributor

Choose a reason for hiding this comment

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

This could have been done by simply changing to

return list(search_smart_split(cleaned_value))

There is no point to changing the return value and definition of search_smart_split.

@sevdog
Copy link
Contributor

sevdog commented Mar 22, 2024

PS: an anti-regression test should be implemented to avoid this, since both older and new versions passed the same tests without any failure.

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