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

Remove doctype from search #5121

Merged
merged 5 commits into from
Jan 22, 2019

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jan 16, 2019

I still want to test this, I'm going to set up ES later.

Related #4896 (review)

@stsewd
Copy link
Member Author

stsewd commented Jan 16, 2019

I just tested this with sphinx and mkdocs, everything built normally.

self.conf_dir(version),
'_build', 'json'
)
return json_path
Copy link
Member Author

Choose a reason for hiding this comment

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

So, this is called from process_all_json_files

https://github.com/rtfd/readthedocs.org/blob/f06271b698d26734bf5024590948d41b1613b2e3/readthedocs/search/parse_json.py#L21-L26

And is used in os.walk, if the path doesn't exists (mkdocs for example), it doesn't raise an error, it just returns an empty iterator.

@@ -1350,7 +1344,8 @@ def sync_callback(_, version_pk, commit, *args, **kwargs):
The first argument is the result from previous tasks, which we discard.
"""
fileify(version_pk, commit=commit)
update_search(version_pk, commit=commit)
if kwargs.get('search'):
Copy link
Member Author

Choose a reason for hiding this comment

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

This shouldn't be needed bc the search code was updated too, but just in case.

@ericholscher
Copy link
Member

I will try and test this locally, but it looks good at first look.

@agjohnson agjohnson added this to the 2.10 milestone Jan 22, 2019
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.

Fixed up the search passing arg, and things look good locally 👍

@@ -741,6 +741,7 @@ def update_app_instances(
callback=sync_callback.s(
version_pk=self.version.pk,
commit=self.build['commit'],
kwargs={'search': search},
Copy link
Member

Choose a reason for hiding this comment

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

This is ending up:

> /Users/eric/projects/readthedocs.org/readthedocs/projects/tasks.py(1395)sync_callback()
   1394     import ipdb; ipdb.set_trace()
-> 1395     if kwargs.get('search'):
   1396         update_search(version_pk, commit=commit)

ipdb> kwargs
{'kwargs': {'search': True}}

Copy link
Member

Choose a reason for hiding this comment

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

Fixing in a push.

@ericholscher
Copy link
Member

Woops. Also ended up pushing my #5150 since I had to have it to test the branch. After that is merged, this should look normal again :)

@ericholscher
Copy link
Member

Mergable when tests pass 👍

@stsewd stsewd merged commit 44a4fe3 into readthedocs:master Jan 22, 2019
@stsewd stsewd deleted the remove-doctype-from-search branch January 22, 2019 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants