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

Projects
None yet
2 participants
@stsewd
Member

stsewd commented Jun 19, 2018

No description provided.

stsewd added some commits Jun 19, 2018

@stsewd

This comment has been minimized.

Member

stsewd commented Jun 20, 2018

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

@agjohnson

@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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

stsewd commented Jun 20, 2018

mmm, do you mean this change rtfd/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 added some commits Jun 21, 2018

@stsewd

This comment has been minimized.

Member

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

@agjohnson

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',

This comment has been minimized.

@agjohnson

agjohnson Jun 25, 2018

Contributor

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

This comment has been minimized.

@stsewd

stsewd Jun 25, 2018

Member

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

This comment has been minimized.

Contributor

agjohnson commented Jun 25, 2018

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

@stsewd

This comment has been minimized.

Member

stsewd commented Jun 25, 2018

Yeah, we can merge this now.

@agjohnson agjohnson merged commit 43073b1 into rtfd:master Jun 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd: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