Skip to content

Remove logic around finding a configuration file inside directories #4714

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

Remove logic around finding a configuration file inside directories #4714

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
16aad41
Remove logic for iterating dirs to find config file
srikant-ch5 Oct 3, 2018
95733b7
Remove functionality of iterating all directories and fixed tests
srikant-ch5 Oct 5, 2018
4c0a670
Remove python debugger import
srikant-ch5 Oct 6, 2018
b1c5d48
Modify tests and remove use of abs path
srikant-ch5 Oct 9, 2018
ba58ea0
Include extra case to config is not searched in nested files
srikant-ch5 Oct 9, 2018
06ee692
Add nested files case through decorator
srikant-ch5 Oct 10, 2018
3e0831e
Update test_config.py
srikant-ch5 Oct 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 5 additions & 12 deletions readthedocs/config/find.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,11 @@
import re


def find_all(path, filename_regex):
"""Find all files in ``path`` that match ``filename_regex`` regex."""
path = os.path.abspath(path)
for root, dirs, files in os.walk(path, topdown=True):
dirs.sort()
for filename in files:
if re.match(filename_regex, filename):
yield os.path.abspath(os.path.join(root, filename))


def find_one(path, filename_regex):
"""Find the first file in ``path`` that match ``filename_regex`` regex."""
for _path in find_all(path, filename_regex):
return _path
_path = os.path.abspath(path)
for filename in os.listdir(_path):
if re.match(filename_regex, filename):
return os.path.join(_path, filename)

return ''
43 changes: 26 additions & 17 deletions readthedocs/config/tests/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,22 @@ def test_load_no_config_file(tmpdir, files):
base = str(tmpdir)
with raises(ConfigError) as e:
load(base, env_config)
nested_files = {
'first': {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case should be moved to the decorator, not inside the function

'readthedocs.yml': '',
},
'second': {
'confuser.txt': 'content',
},
'third': {
'readthedocs.yml': 'content',
'Makefile': '',
},
}
apply_fs(tmpdir, nested_files)
with raises(ConfigError) as e:
load(base, env_config)

assert e.value.code == CONFIG_REQUIRED


Expand Down Expand Up @@ -275,7 +291,6 @@ def test_python_pip_install_default():


def describe_validate_python_extra_requirements():

def it_defaults_to_list():
build = get_build_config({'python': {}}, get_env_config())
build.validate()
Expand Down Expand Up @@ -309,7 +324,6 @@ def it_uses_validate_string(validate_string):


def describe_validate_use_system_site_packages():

def it_defaults_to_false():
build = get_build_config({'python': {}}, get_env_config())
build.validate()
Expand Down Expand Up @@ -337,7 +351,6 @@ def it_uses_validate_bool(validate_bool):


def describe_validate_setup_py_install():

def it_defaults_to_false():
build = get_build_config({'python': {}}, get_env_config())
build.validate()
Expand Down Expand Up @@ -365,7 +378,6 @@ def it_uses_validate_bool(validate_bool):


def describe_validate_python_version():

def it_defaults_to_a_valid_version():
build = get_build_config({'python': {}}, get_env_config())
build.validate()
Expand Down Expand Up @@ -465,7 +477,6 @@ def it_respects_default_value(value):


def describe_validate_formats():

def it_defaults_to_empty():
build = get_build_config({}, get_env_config())
build.validate()
Expand Down Expand Up @@ -545,7 +556,6 @@ def test_valid_build_config():


def describe_validate_base():

def it_validates_to_abspath(tmpdir):
apply_fs(tmpdir, {'configs': minimal_config, 'docs': {}})
with tmpdir.as_cwd():
Expand Down Expand Up @@ -597,7 +607,6 @@ def it_fails_if_base_does_not_exist(tmpdir):


def describe_validate_build():

def it_fails_if_build_is_invalid_option(tmpdir):
apply_fs(tmpdir, minimal_config)
build = BuildConfigV1(
Expand Down Expand Up @@ -1089,12 +1098,12 @@ def test_python_requirements_default_value(self):
assert build.python.requirements is None

def test_python_requirements_allow_null(self):
build = self.get_build_config({'python': {'requirements': None}},)
build = self.get_build_config({'python': {'requirements': None}}, )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These modifications don't seem to belong to this PR

build.validate()
assert build.python.requirements is None

def test_python_requirements_allow_empty_string(self):
build = self.get_build_config({'python': {'requirements': ''}},)
build = self.get_build_config({'python': {'requirements': ''}}, )
build.validate()
assert build.python.requirements == ''

Expand All @@ -1120,13 +1129,13 @@ def test_python_requirements_priority_over_default(self, tmpdir):

@pytest.mark.parametrize('value', [3, [], {}])
def test_python_requirements_check_invalid_types(self, value):
build = self.get_build_config({'python': {'requirements': value}},)
build = self.get_build_config({'python': {'requirements': value}}, )
with raises(InvalidConfig) as excinfo:
build.validate()
assert excinfo.value.key == 'python.requirements'

def test_python_install_pip_check_valid(self):
build = self.get_build_config({'python': {'install': 'pip'}},)
build = self.get_build_config({'python': {'install': 'pip'}}, )
build.validate()
assert build.python.install_with_pip is True
assert build.python.install_with_setup is False
Expand All @@ -1141,7 +1150,7 @@ def test_python_install_pip_priority_over_default(self):
assert build.python.install_with_setup is False

def test_python_install_setuppy_check_valid(self):
build = self.get_build_config({'python': {'install': 'setup.py'}},)
build = self.get_build_config({'python': {'install': 'setup.py'}}, )
build.validate()
assert build.python.install_with_setup is True
assert build.python.install_with_pip is False
Expand All @@ -1166,13 +1175,13 @@ def test_python_install_setuppy_priority_over_default(self):

@pytest.mark.parametrize('value', ['invalid', 'apt'])
def test_python_install_check_invalid(self, value):
build = self.get_build_config({'python': {'install': value}},)
build = self.get_build_config({'python': {'install': value}}, )
with raises(InvalidConfig) as excinfo:
build.validate()
assert excinfo.value.key == 'python.install'

def test_python_install_allow_null(self):
build = self.get_build_config({'python': {'install': None}},)
build = self.get_build_config({'python': {'install': None}}, )
build.validate()
assert build.python.install_with_pip is False
assert build.python.install_with_setup is False
Expand All @@ -1185,7 +1194,7 @@ def test_python_install_default(self):

@pytest.mark.parametrize('value', [2, [], {}])
def test_python_install_check_invalid_type(self, value):
build = self.get_build_config({'python': {'install': value}},)
build = self.get_build_config({'python': {'install': value}}, )
with raises(InvalidConfig) as excinfo:
build.validate()
assert excinfo.value.key == 'python.install'
Expand Down Expand Up @@ -1379,7 +1388,7 @@ def test_sphinx_cant_be_used_with_mkdocs(self, tmpdir):
assert excinfo.value.key == '.'

def test_sphinx_configuration_allow_null(self):
build = self.get_build_config({'sphinx': {'configuration': None}},)
build = self.get_build_config({'sphinx': {'configuration': None}}, )
build.validate()
assert build.sphinx.configuration is None

Expand Down Expand Up @@ -1420,7 +1429,7 @@ def test_sphinx_configuration_priorities_over_default(self, tmpdir):

@pytest.mark.parametrize('value', [[], True, 0, {}])
def test_sphinx_configuration_validate_type(self, value):
build = self.get_build_config({'sphinx': {'configuration': value}},)
build = self.get_build_config({'sphinx': {'configuration': value}}, )
with raises(InvalidConfig) as excinfo:
build.validate()
assert excinfo.value.key == 'sphinx.configuration'
Expand Down
63 changes: 5 additions & 58 deletions readthedocs/config/tests/test_find.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,77 +1,24 @@
from __future__ import division, print_function, unicode_literals

import os

import pytest
import six

from readthedocs.config.find import find_all, find_one
from readthedocs.config.find import find_one

from .utils import apply_fs


def test_find_no_files(tmpdir):
with tmpdir.as_cwd():
paths = list(find_all(os.getcwd(), r'readthedocs.yml'))
assert len(paths) == 0
paths = str(find_one(os.getcwd(), r'readthedocs.yml'))
assert paths == ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to call str here, and we should rename the var name to just path



def test_find_at_root(tmpdir):
apply_fs(tmpdir, {'readthedocs.yml': '', 'otherfile.txt': ''})

base = str(tmpdir)
paths = list(find_all(base, r'readthedocs\.yml'))
assert paths == [
os.path.abspath(os.path.join(base, 'readthedocs.yml'))
]


def test_find_nested(tmpdir):
Copy link
Member

@stsewd stsewd Oct 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd left this test to check that we are not finding nested files

apply_fs(tmpdir, {
'first': {
'readthedocs.yml': '',
},
'second': {
'confuser.txt': 'content',
},
'third': {
'readthedocs.yml': 'content',
'Makefile': '',
},
})
apply_fs(tmpdir, {'first/readthedocs.yml': ''})

base = str(tmpdir)
paths = set(find_all(base, r'readthedocs\.yml'))
assert paths == {
str(tmpdir.join('first', 'readthedocs.yml')),
str(tmpdir.join('third', 'readthedocs.yml')),
}


def test_find_multiple_files(tmpdir):
apply_fs(tmpdir, {
'first': {
'readthedocs.yml': '',
'.readthedocs.yml': 'content',
},
'second': {
'confuser.txt': 'content',
},
'third': {
'readthedocs.yml': 'content',
'Makefile': '',
},
})
apply_fs(tmpdir, {'first/readthedocs.yml': ''})

base = str(tmpdir)
paths = set(find_all(base, r'\.?readthedocs\.yml'))
assert paths == {
str(tmpdir.join('first', 'readthedocs.yml')),
str(tmpdir.join('first', '.readthedocs.yml')),
str(tmpdir.join('third', 'readthedocs.yml')),
}
path = find_one(base, r'readthedocs\.yml')
assert path == os.path.abspath(os.path.join(base, 'readthedocs.yml'))


@pytest.mark.skipif(not six.PY2, reason='Only for python2')
Expand Down