Move config.py from rtd build#4272
Conversation
|
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? |
There was a problem hiding this 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.
|
@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. |
Then this PR is done, I'm reading some patterns right now |
|
We probably still need to address As long as the features are backwards compatible on v1, it's not a problem to support them in v1 i think. |
|
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 |
|
I think I understand the issue, I had added some tests and keep the current behavior for the |
agjohnson
left a comment
agjohnson
left a comment
There was a problem hiding this 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', |
There was a problem hiding this comment.
it looks like we lost none in this list, do we need it?
There was a problem hiding this comment.
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.
|
Is this ready to merge then? It's marked as WIP. |
|
Yeah, we can merge this now. |
stsewd
removed
the
PR: work in progress
2 participants
No description provided.