Add submodule configuration #47
Conversation
| source_file = tmpdir.join('subdir', 'readthedocs.yml') | ||
| build = get_build_config({}, source_file=str(source_file)) | ||
| build.validate_submodules() | ||
| # TODO: This will be the dafult behavior? |
There was a problem hiding this comment.
What I mean here is: on other parts of the code, when the key is not given, it doesn't get added to the build dict, and then the default values are chosen by rtd, I think with that current behavior we are repeating some logic.
| with tmpdir.as_cwd(): | ||
| build = get_build_config({ | ||
| 'submodules': { | ||
| 'exclude': 'all', |
There was a problem hiding this comment.
If when submodules key is not present we are defaulting to include all of them, we need to support exclude: all --otherwise, how we say that we don't want to checkout submodules>
There was a problem hiding this comment.
We do it with include: []
There was a problem hiding this comment.
Still would be necessary exclude: all? I think it's more clear with the other form only.
There was a problem hiding this comment.
To me, include: [] doesn't have a good meaning and it's confusing. It's more explicit to say exclude: all from my understanding.
| assert build['submodules']['include'] == [] | ||
| assert build['submodules']['recursive'] is False | ||
|
|
||
| def it_parses_explicit_wrong_defaults(tmpdir): |
There was a problem hiding this comment.
What would be the behavior for this case? Should be an error? Ignore this? or change the value of recursive to False (this is implemented on this test case)?
stsewd
force-pushed
the
submodule-implementation
branch
from
a287a17 to
8115e5e
Compare
|
|
||
|
|
||
| __all__ = ( | ||
| 'load', 'BuildConfig', 'ConfigError', 'InvalidConfig', 'ProjectConfig') | ||
|
|
||
|
|
||
| CONFIG_FILENAMES = ('readthedocs.yml', '.readthedocs.yml') | ||
| ALL = 'all' |
There was a problem hiding this comment.
It's fine this constant here?
| @@ -168,6 +172,7 @@ def validate(self): | |||
| self.validate_base() | |||
| self.validate_python() | |||
| self.validate_formats() | |||
| self.validate_submodules() | |||
There was a problem hiding this comment.
It's fine triggering the validation here?
| else: | ||
| submodules['include'] = validate_list(include) | ||
| self.validate_directories(submodules['include']) | ||
| if not submodules['include']: |
There was a problem hiding this comment.
Forcing to be always false if include: []
| ) | ||
| return True | ||
|
|
||
| def get_default_submodules_config(self): |
There was a problem hiding this comment.
This would be different for the previous projects
submodules:
include: all
recursive: trueAnd probably this would be managed by the rtd code...
|
@agjohnson should we merge this first? Or maybe port this once the repo is moved? (I think the second option is better since we can really test this) |
|
I agreed that porting over after the merge makes the most sense. I'll set this to blocking for now, but feel free to move over the PR whenever things are ported to rtd core. |
|
This PR was ported to the another repo in readthedocs/readthedocs.org#4355 |
Closes #30 and probably also closes others issues on the rtd repo.
I'm adding only tests for now, hope the behavior for submodules is fine.The checklist: