-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Allow for period as a prefix and yaml extension for config file #4512
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look great! Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I just left some comments about the code style.
readthedocs/config/config.py
Outdated
code=CONFIG_REQUIRED, | ||
) | ||
raise ConfigError('No configuration file found', | ||
code=CONFIG_REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our style guide is something like
raise ConfigError(
'Text',
code=CONFIG_REQUIRED
)
@@ -137,6 +145,14 @@ def test_load_unknow_version(tmpdir): | |||
assert excinfo.value.code == VERSION_INVALID | |||
|
|||
|
|||
def test_yaml_extension(tmpdir): | |||
""" Make sure it's capable of loading the 'readthedocs' file with a 'yaml' extension. """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are spaces in the sides of this docstring
@@ -790,6 +806,13 @@ def test_raise_config_not_supported(): | |||
assert excinfo.value.code == CONFIG_NOT_SUPPORTED | |||
|
|||
|
|||
@pytest.mark.parametrize("correct_config_filename", | |||
[prefix + "readthedocs." + extension for prefix in {"", "."} | |||
for extension in {"yml", "yaml"}]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use singlequotes
Also, there is a test that needs to be changed https://travis-ci.org/rtfd/readthedocs.org/jobs/415587982#L970 |
Sorry this took me a while! Hopefully this should pass now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -1620,3 +1643,6 @@ def test_submodules_recursive_explict_default(self): | |||
assert build.submodules.include == [] | |||
assert build.submodules.exclude == [] | |||
assert build.submodules.recursive is False | |||
|
|||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this extra lines shouldn't be here
Looks great! Thanks a ton for porting the PR over! |
Fixes #4102 bringing it over from the build repo.
Ref. readthedocs/readthedocs-build#48