-
Notifications
You must be signed in to change notification settings - Fork 804
Fix suggest query names for Completion suggestor. #1098
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
Conversation
|
Is there any problem with this PR? thx :) |
honzakral
left a comment
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.
Thank you for the PR, and for the ping. Nothing wrong with the PR except for my lack of time :)
I added a short comment where I think we can do a bit better for easier code, if you agree then we can merge the PR.
| """ | ||
| if text is None and regex is None: | ||
| raise ValueError('You have to pass "text" or "regex" argument.') |
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.
I think we need additional validation rules here - if you supply both text and regex or you supply regex and completion is not in kwargs etc - any invalida combination essentially should raise a ValueError.
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.
OK, i'll fix it more strictly.
elasticsearch_dsl/search.py
Outdated
| query_name= 'regex' | ||
| query_text = regex | ||
|
|
||
| s._suggest[name] = {query_name: query_text} |
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.
once we have validated out all the invalid combinations we should be ok with simpler code:
d = s._suggest[name] = kwargs
if text:
d['text'] = text
else:
d['regex'] = regex|
Thank you for your review. I pray that you can afford the time :) |
|
Can you review this PR again ? |
|
@honzakral Any chance you can review and merge this? |
|
Closing this PR as it's gone stale, rebase on master and I'll review again |
…) (#1836) * Add search_analyzer. * Fix suggest query names for Completion suggestor. * Fix that arguments validation is more strictly. * refactor: add type hints to wrappers.py * Revert "refactor: add type hints to wrappers.py" This reverts commit 90f43ca. * feat: readd regex implementation --------- Co-authored-by: tell-k <[email protected]> Co-authored-by: Miguel Grinberg <[email protected]>
…) (#1836) * Add search_analyzer. * Fix suggest query names for Completion suggestor. * Fix that arguments validation is more strictly. * refactor: add type hints to wrappers.py * Revert "refactor: add type hints to wrappers.py" This reverts commit 90f43ca. * feat: readd regex implementation --------- Co-authored-by: tell-k <[email protected]> Co-authored-by: Miguel Grinberg <[email protected]> (cherry picked from commit 25dfc31)
…) (#1836) (#1840) * Add search_analyzer. * Fix suggest query names for Completion suggestor. * Fix that arguments validation is more strictly. * refactor: add type hints to wrappers.py * Revert "refactor: add type hints to wrappers.py" This reverts commit 90f43ca. * feat: readd regex implementation --------- Co-authored-by: tell-k <[email protected]> Co-authored-by: Miguel Grinberg <[email protected]> (cherry picked from commit 25dfc31) Co-authored-by: Caio Fontes <[email protected]>
According to official documentation, Completion Suggester seems to be using correct
prefixrather thantext.In the
Search.suggestmethod, however, it always becomestext, so I fixed it.In Completion Suggestor you can also specify
regex.I also modified it so that the user can select it.
Please feel free to merge it. thx.