-
Notifications
You must be signed in to change notification settings - Fork 25
Check for readthedocs file with yaml extension #48
Changes from 5 commits
f044969
9c000fb
f1fa2db
d4c888b
d87677c
887e0ea
cc2ef31
0f1f4a6
ec96e3b
afffb2e
1b1ae9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
name: docs | ||
type: sphinx |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ | |
'load', 'BuildConfig', 'ConfigError', 'InvalidConfig', 'ProjectConfig') | ||
|
||
|
||
CONFIG_FILENAMES = ('readthedocs.yml', '.readthedocs.yml') | ||
CONFIG_FILENAME_REGEX = r'\.?readthedocs.ya?ml' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think testing matching is enough |
||
|
||
|
||
BASE_INVALID = 'base-invalid' | ||
|
@@ -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 = { | ||
|
@@ -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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
code=CONFIG_REQUIRED) | ||
build_configs = [] | ||
with open(filename, 'r') as file: | ||
|
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 '' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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')) | ||
] | ||
|
@@ -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'))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can omit the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
@@ -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'))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
|
@@ -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' |
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.
Looks like we aren't using this file, do we?
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.
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.
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.
The
apply_fs
function takes the dict and creates a filesystem with dirs and files on a temporal location.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.
So, the files inside
integration_tests/
are used in other testsThere 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.
@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 addedreadthedocs.yaml
to this folder and created the test to take this file and see if it's found. Am I missing something? ThanksThere 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.
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 inminimal_project
isn't necessary for this test.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 for explaining! I'll give it a quick try pre-work :)
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.
@stsewd tests passes :)