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

Move search artifacts in html step #4213

Closed
wants to merge 4 commits into from

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jun 8, 2018

This together with readthedocs/readthedocs-sphinx-ext#43 closes #3393.

Still wip, I need to remove the previous json builder, and update the rtd extension.

@stsewd stsewd added the PR: work in progress Pull request is not ready for full review label Jun 8, 2018
@stsewd
Copy link
Member Author

stsewd commented Jun 11, 2018

I think this is kind of ready for a review, for test locally you need to replace this line https://github.com/stsewd/readthedocs.org/blob/ffe2d55c15bb8766e00ace3bad693f1b649ecf57/readthedocs/doc_builder/python_environments.py#L249-L249 for git+https://github.com/stsewd/readthedocs-sphinx-ext.git@add-json-process

If what I'm doing is fine, probably we'll need to remove all the search args from the methods (neither mkdocs uses this #4205)

I'm having doubts with readthedocs/readthedocs-sphinx-ext#43 (comment), also I'm not very sure how to test this (it is already tested in the other repo), I suppose I can test that the _json dir is ignored atleast.

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 looks like a good PR for closing out the JSON stuff. I do think we should probably take a staged approach to deploying, which looks like:

  • Ship the JSON output in the extension with a config option to enable it
  • Ship the .org with a feature flag that turns on the JSON extension output config & disables the normal JSON builder
  • Test this with a few projects w/ the feature flag, scaling things up and monitoring the logs to see if this makes things explode
  • Eventually scale the feature flag up to all projects
  • Then remove the existing JSON builder & integration into RTD for it

@stsewd
Copy link
Member Author

stsewd commented Jun 12, 2018

Ok. got it. I'll see if this PR can be reused or making a new one makes sense.

@stsewd
Copy link
Member Author

stsewd commented Jun 12, 2018

Ok, closing this PR most of the changes aren't necessary with the new decision.

@stsewd stsewd closed this Jun 12, 2018
@stsewd stsewd deleted the move-json-process branch June 13, 2018 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: work in progress Pull request is not ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port over JSON builder avoidance from pydoc
2 participants