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 config.py from rtd build #4272

Merged
merged 12 commits into from Jun 25, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Jun 19, 2018

No description provided.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 20, 2018

I think this is ready, next I think I can port the PRs from rtd-build (readthedocs/readthedocs-build#47 and readthedocs/readthedocs-build#38). Next readthedocs/readthedocs-build#50 (comment). @agjohnson what do you think?

Copy link
Contributor

@agjohnson agjohnson left a comment

@stsewd so this might be where we need to be careful. It looks like the formats has changed, but this would be backwards incompatible with our implied version 1 of the schema. I think before we make this change we should work out a pattern to support both v1 and v2. If we still support formats: none, then this is not an issue though

Perhaps for now we should just port the code, get it running in RTD core, then start working out this pattern, then start porting in some of the backwards incompatible changes.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 20, 2018

@agjohnson I was thinking about porting the rest of the PRs to v1, then work in the support for the two versions and v2 features. I'm not sure how much code will change to support the two versions p:. I will investigate some patterns and see what can fit here.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 20, 2018

Perhaps for now we should just port the code, get it running in RTD core, then start working out this pattern, then start porting in some of the backwards incompatible changes.

Then this PR is done, I'm reading some patterns right now

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Jun 20, 2018

We probably still need to address formats in this PR though, as that is going to break for existing yaml configurations with this PR. You could maybe remove the changes to formats and we can merge this.

As long as the features are backwards compatible on v1, it's not a problem to support them in v1 i think.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 20, 2018

mmm, do you mean this change readthedocs/readthedocs-build#43? I thought it was already deployed. We already change the docs for that https://docs.readthedocs.io/en/latest/yaml-config.html#formats. Also, the value was never used in the rtd code. But it wasn't causing any error in both sides anyway. Should I keep the compatibility with formats: none?

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 21, 2018

I think I understand the issue, I had added some tests and keep the current behavior for the formats key.

@stsewd stsewd requested a review from agjohnson Jun 24, 2018
Copy link
Contributor

@agjohnson agjohnson left a comment

I'm confused why the test case works currently, but if we don't need none in this list, then 👍 on this.

def get_valid_formats(self): # noqa
"""Get all valid documentation formats."""
return (
'htmlzip',
Copy link
Contributor

@agjohnson agjohnson Jun 25, 2018

it looks like we lost none in this list, do we need it?

Copy link
Member Author

@stsewd stsewd Jun 25, 2018

I moved this logic, it doesn't make much sense having this as a valid option, but in the past, we allowed ['none'] as a way of ignoring all the types, this check is now in https://github.com/rtfd/readthedocs.org/pull/4272/files#diff-2b6854693f39acfa183fca7fef27dcfbR423. BTW, we never put none as a valid option in the past docs.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Jun 25, 2018

Is this ready to merge then? It's marked as WIP.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 25, 2018

Yeah, we can merge this now.

@agjohnson agjohnson merged commit 43073b1 into readthedocs:master Jun 25, 2018
1 check passed
@stsewd stsewd deleted the move-configpy-from-rtd-build branch Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants