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

Add submodule configuration #47

Closed
wants to merge 5 commits into from
Closed

Add submodule configuration #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8527520
Add tests for submodules
stsewd Mar 22, 2018
85989fb
Add more tests
stsewd Mar 22, 2018
30d66e4
Fix tests
stsewd Mar 23, 2018
8115e5e
Validate submodules
stsewd Mar 23, 2018
6cffaeb
Allow ALL keyword on submodules.exclude key
stsewd Mar 31, 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
67 changes: 66 additions & 1 deletion readthedocs_build/config/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,23 @@
from .parser import ParseError
from .parser import parse
from .validation import (validate_bool, validate_choice, validate_directory,
validate_file, validate_string, ValidationError)
validate_file, validate_list, validate_string,
ValidationError)


__all__ = (
'load', 'BuildConfig', 'ConfigError', 'InvalidConfig', 'ProjectConfig')


CONFIG_FILENAMES = ('readthedocs.yml', '.readthedocs.yml')
ALL = 'all'
Copy link
Member Author

Choose a reason for hiding this comment

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

It's fine this constant here?



BASE_INVALID = 'base-invalid'
BASE_NOT_A_DIR = 'base-not-a-directory'
CONFIG_SYNTAX_INVALID = 'config-syntax-invalid'
INVALID_KEYS_COMBINATION = 'invalid-keys-combination'
MISSING_REQUIRED_KEY = 'missing-required-key'
CONFIG_REQUIRED = 'config-required'
NAME_REQUIRED = 'name-required'
NAME_INVALID = 'name-invalid'
Expand Down Expand Up @@ -168,6 +172,7 @@ def validate(self):
self.validate_base()
self.validate_python()
self.validate_formats()
self.validate_submodules()
Copy link
Member Author

Choose a reason for hiding this comment

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

It's fine triggering the validation here?


self.validate_conda()
self.validate_requirements_file()
Expand Down Expand Up @@ -408,6 +413,66 @@ def validate_formats(self):

return True

def validate_submodules(self):
raw_submodules = self.raw_config.get('submodules')
if raw_submodules is None:
self['submodules'] = self.get_default_submodules_config()
return None

if 'include' in raw_submodules and 'exclude' in raw_submodules:
self.error(
'submodules',
'It is not possible to include and exclude submodules',
code=INVALID_KEYS_COMBINATION
)

self['submodules'] = {}
submodules = self['submodules']

with self.catch_validation_error('submodules.recursive'):
recursive = raw_submodules.get('recursive', False)
submodules['recursive'] = validate_bool(recursive)

if 'include' in raw_submodules:
with self.catch_validation_error('submodules.include'):
include = raw_submodules['include']
if include == ALL:
submodules['include'] = ALL
else:
submodules['include'] = validate_list(include)
self.validate_directories(submodules['include'])
if not submodules['include']:
Copy link
Member Author

Choose a reason for hiding this comment

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

Forcing to be always false if include: []

submodules['recursive'] = False
elif 'exclude' in raw_submodules:
with self.catch_validation_error('submodules.exclude'):
exclude = raw_submodules['exclude']
if exclude == ALL:
submodules['exclude'] = ALL
submodules['recursive'] = False
else:
submodules['exclude'] = validate_list(exclude)
self.validate_directories(submodules['exclude'])
elif submodules['recursive'] is True:
self.error(
'submodules',
'You need to include submodules',
code=MISSING_REQUIRED_KEY
)
return True

def get_default_submodules_config(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

This would be different for the previous projects

submodules:
  include: all
  recursive: true

And probably this would be managed by the rtd code...

return {
'include': [],
'recursive': False,
}

def validate_directories(self, directories):
base_path = os.path.dirname(self.source_file)
return [
validate_directory(directory, base_path)
for directory in directories
]


class ProjectConfig(list):

Expand Down
202 changes: 202 additions & 0 deletions readthedocs_build/config/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,17 @@
from .config import load
from .config import BuildConfig
from .config import ProjectConfig
from .config import ALL
from .config import INVALID_KEYS_COMBINATION
from .config import MISSING_REQUIRED_KEY
from .config import TYPE_REQUIRED
from .config import NAME_REQUIRED
from .config import NAME_INVALID
from .config import PYTHON_INVALID
from .validation import INVALID_BOOL
from .validation import INVALID_CHOICE
from .validation import INVALID_DIRECTORY
from .validation import INVALID_LIST
from .validation import INVALID_PATH
from .validation import INVALID_STRING

Expand Down Expand Up @@ -349,6 +353,204 @@ def it_uses_validate_file(tmpdir):
assert args[0] == 'setup.py'


def describe_validate_submodules():

def it_defaults_to_all_submodules(tmpdir):
with tmpdir.as_cwd():
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?
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

assert 'submodules' in build
assert build['submodules']['include'] == []
assert build['submodules']['recursive'] is False

def it_parses_include_key(tmpdir):
apply_fs(tmpdir, {'sub-one': {}, 'sub-two': {}})
with tmpdir.as_cwd():
build = get_build_config({
'submodules': {
'include': ['sub-one/', 'sub-two'],
}
})
build.validate_submodules()
assert 'submodules' in build
assert 'exclude' not in build['submodules']
assert build['submodules']['include'] == ['sub-one/', 'sub-two']
assert build['submodules']['recursive'] is False

def it_validates_include_key(tmpdir):
apply_fs(tmpdir, {'sub-one': {}, 'sub-two': {}})
with tmpdir.as_cwd():
build = get_build_config({
'submodules': {
'include': ['sub-one/', 'sub-too'],
}
})
with raises(InvalidConfig) as excinfo:
build.validate_submodules()
assert excinfo.value.key == 'submodules.include'
assert excinfo.value.code == INVALID_PATH

def it_parses_exclude_key(tmpdir):
apply_fs(tmpdir, {'sub-one': {}, 'sub-two': {}})
with tmpdir.as_cwd():
build = get_build_config({
'submodules': {
'exclude': ['sub-two'],
}
})
build.validate_submodules()
assert 'submodules' in build
assert 'include' not in build['submodules']
assert build['submodules']['exclude'] == ['sub-two']
assert build['submodules']['recursive'] is False

def it_validates_exclude_key(tmpdir):
apply_fs(tmpdir, {'sub-one': {}, 'sub-two': {}})
with tmpdir.as_cwd():
build = get_build_config({
'submodules': {
'exclude': ['sub-too'],
}
})
with raises(InvalidConfig) as excinfo:
build.validate_submodules()
assert excinfo.value.key == 'submodules.exclude'
assert excinfo.value.code == INVALID_PATH

def it_does_not_allow_exclude_and_include(tmpdir):
build = get_build_config({
'submodules': {
'include': ['sub-one'],
'exclude': ['sub-two'],
}
})
with raises(InvalidConfig) as excinfo:
build.validate_submodules()
assert excinfo.value.key == 'submodules'
assert excinfo.value.code == INVALID_KEYS_COMBINATION

def it_requires_include_with_recursive(tmpdir):
build = get_build_config({
'submodules': {
'recursive': True,
}
})
with raises(InvalidConfig) as excinfo:
build.validate_submodules()
assert excinfo.value.key == 'submodules'
assert excinfo.value.code == MISSING_REQUIRED_KEY

def it_parses_recursive_key(tmpdir):
apply_fs(tmpdir, {'sub-one': {}, 'sub-two': {}})
with tmpdir.as_cwd():
build = get_build_config({
'submodules': {
'exclude': ['sub-two'],
'recursive': True,
},
})
build.validate_submodules()
assert build['submodules']['recursive'] is True

def it_validates_recursive_key(tmpdir):
apply_fs(tmpdir, {'sub-one': {}, 'sub-two': {}})
with tmpdir.as_cwd():
build = get_build_config({
'submodules': {
'exclude': ['sub-two'],
'recursive': 'True',
},
})
with raises(InvalidConfig) as excinfo:
build.validate_submodules()
assert excinfo.value.key == 'submodules.recursive'
assert excinfo.value.code == INVALID_BOOL

def it_accepts_all_keyword_on_include(tmpdir):
apply_fs(tmpdir, {'sub-one': {}, 'sub-two': {}})
with tmpdir.as_cwd():
build = get_build_config({
'submodules': {
'include': 'all',
'recursive': True,
},
})
build.validate_submodules()
assert build['submodules']['include'] == ALL
assert build['submodules']['recursive'] is True

def it_validates_the_correct_type_for_include(tmpdir):
apply_fs(tmpdir, {'sub-one': {}, 'sub-two': {}})
with tmpdir.as_cwd():
build = get_build_config({
'submodules': {
'include': 'none',
'recursive': True,
},
})
with raises(InvalidConfig) as excinfo:
build.validate_submodules()
assert excinfo.value.key == 'submodules.include'
assert excinfo.value.code == INVALID_LIST

def it_accepts_all_keyword_on_exclude(tmpdir):
apply_fs(tmpdir, {'sub-one': {}, 'sub-two': {}})
with tmpdir.as_cwd():
build = get_build_config({
'submodules': {
'exclude': 'all',
Copy link
Member

Choose a reason for hiding this comment

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

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>

Copy link
Member Author

Choose a reason for hiding this comment

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

We do it with include: []

Copy link
Member Author

Choose a reason for hiding this comment

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

Still would be necessary exclude: all? I think it's more clear with the other form only.

Copy link
Member

Choose a reason for hiding this comment

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

To me, include: [] doesn't have a good meaning and it's confusing. It's more explicit to say exclude: all from my understanding.

'recursive': True,
},
})
build.validate_submodules()
assert build['submodules']['exclude'] == ALL
assert build['submodules']['recursive'] is False

def it_validates_the_correct_type_for_exclude(tmpdir):
apply_fs(tmpdir, {'sub-one': {}, 'sub-two': {}})
with tmpdir.as_cwd():
build = get_build_config({
'submodules': {
'exclude': 'none',
'recursive': True,
},
})
with raises(InvalidConfig) as excinfo:
build.validate_submodules()
assert excinfo.value.key == 'submodules.exclude'
assert excinfo.value.code == INVALID_LIST

def it_parses_explicit_defaults(tmpdir):
apply_fs(tmpdir, {'sub-one': {}, 'sub-two': {}})
with tmpdir.as_cwd():
build = get_build_config({
'submodules': {
'include': [],
'recursive': False,
}
})
build.validate_submodules()
assert 'submodules' in build
assert build['submodules']['include'] == []
assert build['submodules']['recursive'] is False

def it_parses_explicit_wrong_defaults(tmpdir):
Copy link
Member Author

Choose a reason for hiding this comment

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

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)?

apply_fs(tmpdir, {'sub-one': {}, 'sub-two': {}})
with tmpdir.as_cwd():
build = get_build_config({
'submodules': {
'include': [],
'recursive': True,
}
})
build.validate_submodules()
assert 'submodules' in build
assert build['submodules']['include'] == []
assert build['submodules']['recursive'] is False


def test_valid_build_config():
build = BuildConfig(env_config,
minimal_config,
Expand Down