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

Hotfix: Return empty dict when no highlight dict is present #5950

Merged

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Jul 17, 2019

I have moved the sorting logic to search.utils.

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jul 17, 2019

Ohhh... I just noticed.
We have modified the parse_json to remove all the new-line characters.
If we don't have any new line characters in our index -- then there won't be any new line characters in the highlight.
I think the func _remove_newlines_from_dict() is no longer required.

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jul 17, 2019

This should also increase the speed as that func is going through every highlight dict recursively and that too -- doing nothing.


return highlight
if logging:
log.debug('API Search highlight: %s', pformat(highlight_dict))
Copy link
Member Author

@dojutsu-user dojutsu-user Jul 17, 2019

I also wanted to know that if we can do something for pformat.
@humitos pointed out that it is doing unnecessary work (comment).
There are other places also where we are using pformat for logging purposes.
This must be reducing some speed and taking memory.

Copy link
Member

@ericholscher ericholscher Jul 17, 2019

We can just output it unformatted.

Copy link
Member

@ericholscher ericholscher left a comment

I think we can simplify this a bit.


return highlight
if logging:
log.debug('API Search highlight: %s', pformat(highlight_dict))
Copy link
Member

@ericholscher ericholscher Jul 17, 2019

We can just output it unformatted.

sorted_results = list(utils._get_sorted_results(
results=all_results,
source_key='_source',
logging=True,
Copy link
Member

@ericholscher ericholscher Jul 17, 2019

optional logging feels weird. Do we ever not want logs?

Copy link
Member

@ericholscher ericholscher Jul 17, 2019

I think we should probably just log outside this function, and remove the logging from this function call.

Copy link
Member Author

@dojutsu-user dojutsu-user Jul 17, 2019

Done.


def _get_sorted_results(results, source_key='_source', logging=False):
"""Sort results according to their score and return a generator expression."""
sorted_results = (
Copy link
Member

@ericholscher ericholscher Jul 17, 2019

Do we really need this to be a generator? What is the goal of it versus just using a list? It looks like we're just calling list on it both times we use it, which means it should probably just return a list :)

Copy link
Member Author

@dojutsu-user dojutsu-user Jul 17, 2019

I thought that returning a generator expression would be efficient than returning a huge list of results?

def _remove_newlines_from_dict(highlight):
"""
Recursively change results to turn newlines into periods.
def _get_inner_hits_highlights(hit, logging=False):
Copy link
Member

@ericholscher ericholscher Jul 17, 2019

It seems like maybe we don't even need this function. It's mostly just checking if the hit has a highlight and returning it. We can do that in the parent function itself pretty easily.

Copy link
Member Author

@dojutsu-user dojutsu-user Jul 17, 2019

Removed. 👍

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jul 17, 2019

Looks good once we fix the conflict 👍

@ericholscher ericholscher merged commit 2888d2e into readthedocs:master Jul 17, 2019
1 check passed
@dojutsu-user dojutsu-user deleted the fix--invalid-highlight-search branch Jul 17, 2019
@dojutsu-user dojutsu-user moved this from In progress to Done in In-doc search UI Jul 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants