Skip to content
This repository was archived by the owner on Mar 18, 2022. It is now read-only.

Check for readthedocs file with yaml extension #48

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
4 changes: 2 additions & 2 deletions docs/spec.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ Creating a build works like this:

``rtd-build`` will then perform the following actions:

- it searches for all ``readthedocs.yml`` files below the current directory
and merges all found files into a list of configurations
- it searches for all ``readthedocs.yml``, ``.readthedocs.yml``, or ``readthedocs.yaml`` files below the current
directory and merges all found files into a list of configurations
- it iterates over all configurations (order is not garuanteed) and performs
the following actions for each:

Expand Down
2 changes: 2 additions & 0 deletions integration_tests/minimal_project/readthedocs.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
name: docs
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we aren't using this file, do we?

Copy link
Author

Choose a reason for hiding this comment

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

If you look at https://github.com/rtfd/readthedocs-build/pull/48/files#diff-c5258b81d7a5c3182be62b7f82e02aedR105 there is a new test that uses it. Not sure if this is redundant considering the "find" functionality tests.

Copy link
Member

Choose a reason for hiding this comment

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

The apply_fs function takes the dict and creates a filesystem with dirs and files on a temporal location.

Copy link
Member

Choose a reason for hiding this comment

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

So, the files inside integration_tests/ are used in other tests

Copy link
Author

Choose a reason for hiding this comment

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

@stsewd maybe I'm confused: I'm thinking that the apply_fs function makes a temporary directory to test, and because I'd like to test whether the .yaml file is found I've added readthedocs.yaml to this folder and created the test to take this file and see if it's found. Am I missing something? Thanks

Copy link
Member

Choose a reason for hiding this comment

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

I've added readthedocs.yaml to this folder

If you mean by this line
https://github.com/rtfd/readthedocs-build/pull/48/files#diff-c5258b81d7a5c3182be62b7f82e02aedR52

Then, the file that you created isn't necessary, the apply_fs will create that file for you in a temporary directory with the content name: docs\ntype: sphinx. The file in minimal_project isn't necessary for this test.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for explaining! I'll give it a quick try pre-work :)

Copy link
Author

Choose a reason for hiding this comment

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

@stsewd tests passes :)

type: sphinx
12 changes: 4 additions & 8 deletions readthedocs_build/config/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
'load', 'BuildConfig', 'ConfigError', 'InvalidConfig', 'ProjectConfig')


CONFIG_FILENAMES = ('readthedocs.yml', '.readthedocs.yml')
CONFIG_FILENAME_REGEX = r'\.?readthedocs.ya?ml'
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if isn't better to test this regex (because right now we are hardcoding a regex in the tests)

Copy link
Author

Choose a reason for hiding this comment

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

Sure, can do that too, is a very easy test, I'm just wondering what will I actually be testing? I can see 4 permutations that the regex need to test, but should I test that it doesn't match something else as well?

Copy link
Member

Choose a reason for hiding this comment

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

I think testing matching is enough



BASE_INVALID = 'base-invalid'
Expand All @@ -29,7 +29,7 @@

DOCKER_DEFAULT_IMAGE = 'readthedocs/build'
DOCKER_DEFAULT_VERSION = '2.0'
# These map to coordisponding settings in the .org,
# These map to corresponding settings in the .org,
# so they haven't been renamed.
DOCKER_IMAGE = '{}:{}'.format(DOCKER_DEFAULT_IMAGE, DOCKER_DEFAULT_VERSION)
DOCKER_IMAGE_SETTINGS = {
Expand Down Expand Up @@ -433,14 +433,10 @@ def load(path, env_config):
The config will be validated.
"""

filename = find_one(path, CONFIG_FILENAMES)
filename = find_one(path, CONFIG_FILENAME_REGEX)

if not filename:
files = '{}'.format(', '.join(map(repr, CONFIG_FILENAMES[:-1])))
if files:
files += ' or '
files += '{!r}'.format(CONFIG_FILENAMES[-1])
raise ConfigError('No files {} found'.format(files),
raise ConfigError('No files with regex \'{}\' found'.format(CONFIG_FILENAME_REGEX),
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to mention regex here as this is a user facing error. The error message should probably just be "No configuration file found."

Copy link
Author

Choose a reason for hiding this comment

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

Done

code=CONFIG_REQUIRED)
build_configs = []
with open(filename, 'r') as file:
Expand Down
11 changes: 6 additions & 5 deletions readthedocs_build/config/find.py
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
import os
import re


def find_all(path, filenames):
def find_all(path, filename_regex):
path = os.path.abspath(path)
for root, dirs, files in os.walk(path, topdown=True):
dirs.sort()
for filename in filenames:
if filename in files:
for filename in files:
if re.match(filename_regex, filename):
yield os.path.abspath(os.path.join(root, filename))


def find_one(path, filenames):
for _path in find_all(path, filenames):
def find_one(path, filename_regex):
for _path in find_all(path, filename_regex):
return _path
return ''
15 changes: 15 additions & 0 deletions readthedocs_build/config/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@
'''
}

yaml_extension_config_dir = {
'readthedocs.yaml': '''\
name: docs
type: sphinx
'''
}


multiple_config_dir = {
'readthedocs.yml': '''
Expand Down Expand Up @@ -95,6 +102,14 @@ def test_minimal_config(tmpdir):
assert isinstance(build, BuildConfig)


def test_yaml_extension(tmpdir):
""" Make sure it's capable of loading the 'readthedocs' file with a 'yaml' extension. """
apply_fs(tmpdir, yaml_extension_config_dir)
base = str(tmpdir)
config = load(base, env_config)
assert len(config) == 1


def test_build_config_has_source_file(tmpdir):
base = str(apply_fs(tmpdir, minimal_config_dir))
build = load(base, env_config)[0]
Expand Down
27 changes: 9 additions & 18 deletions readthedocs_build/config/test_find.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@

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


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

base = str(tmpdir)
paths = list(find_all(base, ('readthedocs.yml',)))
paths = list(find_all(base, r'readthedocs\.yml'))
assert paths == [
os.path.abspath(os.path.join(base, 'readthedocs.yml'))
]
Expand All @@ -39,11 +39,11 @@ def test_find_nested(tmpdir):
apply_fs(tmpdir, {'first/readthedocs.yml': ''})

base = str(tmpdir)
paths = list(find_all(base, ('readthedocs.yml',)))
assert set(paths) == set([
paths = set(list(find_all(base, r'readthedocs\.yml')))
Copy link
Member

Choose a reason for hiding this comment

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

We can omit the list conversion here and directly do a set

Copy link
Author

Choose a reason for hiding this comment

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

yep

assert paths == {
str(tmpdir.join('first', 'readthedocs.yml')),
str(tmpdir.join('third', 'readthedocs.yml')),
])
}


def test_find_multiple_files(tmpdir):
Expand All @@ -63,21 +63,12 @@ def test_find_multiple_files(tmpdir):
apply_fs(tmpdir, {'first/readthedocs.yml': ''})

base = str(tmpdir)
paths = list(find_all(base, ('readthedocs.yml',
'.readthedocs.yml')))
assert paths == [
paths = set(list(find_all(base, r'\.?readthedocs\.yml')))
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Author

Choose a reason for hiding this comment

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

yep

assert paths == {
str(tmpdir.join('first', 'readthedocs.yml')),
str(tmpdir.join('first', '.readthedocs.yml')),
str(tmpdir.join('third', 'readthedocs.yml')),
]

paths = list(find_all(base, ('.readthedocs.yml',
'readthedocs.yml')))
assert paths == [
str(tmpdir.join('first', '.readthedocs.yml')),
str(tmpdir.join('first', 'readthedocs.yml')),
str(tmpdir.join('third', 'readthedocs.yml')),
]
}


@pytest.mark.skipif(not six.PY2, reason='Only for python2')
Expand All @@ -87,6 +78,6 @@ def test_find_unicode_path(tmpdir):
assert isinstance(base_path, str)
unicode_base_path = base_path.decode('utf-8')
assert isinstance(unicode_base_path, unicode)
path = find_one(unicode_base_path, ('readthedocs.yml',))
path = find_one(unicode_base_path, r'readthedocs\.yml')
assert path == ''
assert False, 'The UnicodeDecodeError was not raised'