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.

Sign up for GitHub

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

Jump to bottom

Search: generate full link from the server side #7070

Merged
merged 8 commits into from
May 14, 2020
Merged

Search: generate full link from the server side #7070

merged 8 commits into from
May 14, 2020

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented May 12, 2020 •
edited
Loading

We were returning an invalid link from the api,
and the client was in charge of guessing the correct link.

This also drops our builder as dependency for search 🎉

Half fixes/ #5821
Fixes #6102

Note: the search as you type extension needs to change as well.

You can test this locally with this project https://2.python-requests.org/en/master/search/?q=%22The+Contributor%22

@stsewd
Search: generate full link from the server side
4c48775 
We were returning an invalid link from the api,
and the cliente was in charge of guessing the correct link.

This also drops our builder as dependency for search 🎉

Half fixex/ #5821
Fixes #6102
@stsewd
Merge branch 'master' into generate-full-link
7fb6f49
stsewd
stsewd commented May 12, 2020
View reviewed changes
readthedocs/search/api.py Outdated Show resolved Hide resolved
@stsewd
Copy link
Member Author

stsewd commented May 12, 2020 •
edited
Loading

I'll add a couple of tests for this tomorrow, looks like we don't have any of them.

stsewd added a commit to readthedocs/readthedocs-sphinx-search that referenced this pull request May 12, 2020
@stsewd
Don't guess the correct link
a127366 
This should be merged after readthedocs/readthedocs.org#7070
is deployed.
@stsewd stsewd mentioned this pull request May 12, 2020
Merged
@stsewd
Copy link
Member Author

stsewd commented May 12, 2020 •
edited
Loading

This readthedocs/readthedocs-sphinx-search#50 should be merged after this PR is deployed

ericholscher
ericholscher reviewed May 13, 2020
View reviewed changes
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This is a good change, waiting on tests for 👍

readthedocs/search/api.py Outdated Show resolved Hide resolved
@stsewd stsewd requested a review from ericholscher May 13, 2020 23:39
ericholscher
ericholscher reviewed May 14, 2020
View reviewed changes
readthedocs/search/api.py Show resolved Hide resolved
readthedocs/search/tests/conftest.py Outdated Show resolved Hide resolved
readthedocs/search/tests/test_api.py Outdated Show resolved Hide resolved
@stsewd stsewd requested a review from ericholscher May 14, 2020 15:30
@stsewd stsewd merged commit bd2f511 into master May 14, 2020
@stsewd stsewd deleted the generate-full-link branch May 14, 2020 22:19
@YuriSizov YuriSizov mentioned this pull request May 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericholscher ericholscher ericholscher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Projects built with htmldir have invalid link for homepage in search results
2 participants
@stsewd @ericholscher