Skip to content
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

Match v1 config interface to new one #4456

Merged
merged 28 commits into from
Aug 24, 2018
Merged

Match v1 config interface to new one #4456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
5a707e9
Add integratio tests for config file v2
stsewd Jul 31, 2018
8b564b6
Complete tests for python
stsewd Aug 1, 2018
c15c539
Change external keys to python scope v1
stsewd Aug 1, 2018
229daf7
Fix tests
stsewd Aug 1, 2018
8edadf0
Adapt tests from v1
stsewd Aug 1, 2018
73731d8
Skip new tests
stsewd Aug 1, 2018
6f6a4cf
Refactor code to use new python options
stsewd Aug 1, 2018
41e5fb7
Refactor conda option from v1
stsewd Aug 1, 2018
14e931a
Use new conda option in tests
stsewd Aug 1, 2018
64df5ed
Linter
stsewd Aug 1, 2018
45c2cff
Use new configurations
stsewd Aug 1, 2018
359c3ff
Add doctype to v1 & move models
stsewd Aug 1, 2018
691f13c
Use build model in v1
stsewd Aug 1, 2018
8361603
Use new key for build.image
stsewd Aug 1, 2018
7e72b91
Stop skipping tests
stsewd Aug 1, 2018
3080632
Test empty requirements
stsewd Aug 1, 2018
4b301b8
Linter
stsewd Aug 1, 2018
c4661d7
Use new settings
stsewd Aug 1, 2018
ec1e077
Add tests for sphinx
stsewd Aug 1, 2018
32c9aeb
Add tests for mkdocs
stsewd Aug 1, 2018
5b37760
Pass config object to builders
stsewd Aug 1, 2018
f1c99dc
Revert "Pass config object to builders"
stsewd Aug 2, 2018
fb74829
Skip new tests
stsewd Aug 2, 2018
0141139
Style
stsewd Aug 2, 2018
5ae10e8
Fix test
stsewd Aug 2, 2018
e73480f
One more test for v2
stsewd Aug 3, 2018
27759fe
Test conf.py default value
stsewd Aug 3, 2018
4baf899
Better check for conda
stsewd Aug 15, 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
125 changes: 31 additions & 94 deletions readthedocs/config/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@

import os
import re
from collections import namedtuple
from contextlib import contextmanager

import six

from .find import find_one
from .models import Build, Conda, Mkdocs, Python, Sphinx, Submodules
from .parser import ParseError, parse
from .validation import (
ValidationError, validate_bool, validate_choice, validate_dict,
Expand Down Expand Up @@ -177,7 +177,7 @@ def python_interpreter(self):

@property
def python_full_version(self):
ver = self.python_version
ver = self.python.version
if ver in [2, 3]:
# Get the highest version of the major series version if user only
# gave us a version of '2', or '3'
Expand Down Expand Up @@ -361,12 +361,9 @@ def validate_python(self):
version = self.defaults.get('python_version', 2)
python = {
'use_system_site_packages': use_system_packages,
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 we should use system_packages as name here

There are more context in another comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shorter is better p:

'pip_install': False,
'install_with_pip': False,
'extra_requirements': [],
'setup_py_install': install_project,
'setup_py_path': os.path.join(
os.path.dirname(self.source_file),
'setup.py'),
'install_with_setup': install_project,
'version': version,
}

Expand All @@ -388,7 +385,7 @@ def validate_python(self):
# Validate pip_install.
if 'pip_install' in raw_python:
with self.catch_validation_error('python.pip_install'):
python['pip_install'] = validate_bool(
python['install_with_pip'] = validate_bool(
raw_python['pip_install'])

# Validate extra_requirements.
Expand All @@ -399,26 +396,22 @@ def validate_python(self):
'python.extra_requirements',
self.PYTHON_EXTRA_REQUIREMENTS_INVALID_MESSAGE,
code=PYTHON_INVALID)
for extra_name in raw_extra_requirements:
with self.catch_validation_error(
'python.extra_requirements'):
python['extra_requirements'].append(
validate_string(extra_name)
)
if not python['install_with_pip']:
python['extra_requirements'] = []
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it should affect, but this checking will be new for V1.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we were doing this validation in the rtd code before.

else:
for extra_name in raw_extra_requirements:
with self.catch_validation_error(
'python.extra_requirements'):
python['extra_requirements'].append(
validate_string(extra_name)
)

# Validate setup_py_install.
if 'setup_py_install' in raw_python:
with self.catch_validation_error('python.setup_py_install'):
python['setup_py_install'] = validate_bool(
python['install_with_setup'] = validate_bool(
raw_python['setup_py_install'])

# Validate setup_py_path.
if 'setup_py_path' in raw_python:
Copy link
Member

Choose a reason for hiding this comment

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

Did we remove this option?

Copy link
Member

Choose a reason for hiding this comment

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

In V1 we were validating that the path to setup.py exists and here we are removing this check.

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 option was never implemented in v1, we are considering implementing this in v2 #4354

with self.catch_validation_error('python.setup_py_path'):
base_path = os.path.dirname(self.source_file)
python['setup_py_path'] = validate_file(
raw_python['setup_py_path'], base_path)

if 'version' in raw_python:
with self.catch_validation_error('python.version'):
# Try to convert strings to an int first, to catch '2', then
Expand Down Expand Up @@ -451,11 +444,14 @@ def validate_conda(self):
self.PYTHON_INVALID_MESSAGE,
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 this message is wrong. It will say

"python" section must be a mapping.

but we are under conda section.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this was ported from the previous config file, I'm not sure if this is in the scope of this PR, but now we have a validate_dict function for this cases.

Copy link
Member

Choose a reason for hiding this comment

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

If it's a problem, and won't be fixed in this PR, please open an issue for this so we don't forget to fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #4530 for this

code=PYTHON_INVALID)

conda_environment = None
if 'file' in raw_conda:
with self.catch_validation_error('conda.file'):
base_path = os.path.dirname(self.source_file)
conda['file'] = validate_file(
raw_conda['file'], base_path)
conda_environment = validate_file(
raw_conda['file'], base_path
)
conda['environment'] = conda_environment

return conda
return None
Expand Down Expand Up @@ -511,58 +507,24 @@ def formats(self):
@property
def python(self):
"""Python related configuration."""
return self._config.get('python', {})

@property
def python_version(self):
"""Python version."""
return self._config['python']['version']

@property
def pip_install(self):
"""True if the project should be installed using pip."""
return self._config['python']['pip_install']

@property
def install_project(self):
"""True if the project should be installed."""
if self.pip_install:
return True
return self._config['python']['setup_py_install']

@property
def extra_requirements(self):
"""Extra requirements to be installed with pip."""
if self.pip_install:
return self._config['python']['extra_requirements']
return []

@property
def use_system_site_packages(self):
"""True if the project should have access to the system packages."""
return self._config['python']['use_system_site_packages']

@property
def use_conda(self):
"""True if the project use Conda."""
return self._config.get('conda') is not None
requirements = self._config['requirements_file']
self._config['python']['requirements'] = requirements
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that self._config['python'] will always exists and be a dictionary? In the init we are not defining it.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I think this method may return None, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we always have a valid dict for self._config['python']. How is that? We always need to call to validate before using any of this properties, there is a docstring about that. The method always returns a valid object

return Python(**self._config['python'])

@property
def conda_file(self):
"""The Conda environment file."""
if self.use_conda:
return self._config['conda'].get('file')
def conda(self):
if self._config['conda'] is not None:
return Conda(**self._config['conda'])
return None

@property
def requirements_file(self):
"""The project requirements file."""
return self._config['requirements_file']
def build(self):
"""The docker image used by the builders."""
return Build(**self._config['build'])

@property
def build_image(self):
"""The docker image used by the builders."""
return self._config['build']['image']
def doctype(self):
return self.defaults['doctype']


class BuildConfigV2(BuildConfigBase):
Expand Down Expand Up @@ -888,47 +850,26 @@ def formats(self):

@property
def conda(self):
Conda = namedtuple('Conda', ['environment']) # noqa
if self._config['conda']:
return Conda(**self._config['conda'])
return None

@property
def build(self):
Build = namedtuple('Build', ['image']) # noqa
return Build(**self._config['build'])

@property
def python(self):
Python = namedtuple( # noqa
'Python',
[
'version',
'requirements',
'install_with_pip',
'install_with_setup',
'extra_requirements',
'use_system_site_packages',
],
)
return Python(**self._config['python'])

@property
def sphinx(self):
Sphinx = namedtuple( # noqa
'Sphinx',
['builder', 'configuration', 'fail_on_warning'],
)
if self._config['sphinx']:
return Sphinx(**self._config['sphinx'])
return None

@property
def mkdocs(self):
Mkdocs = namedtuple( # noqa
'Mkdocs',
['configuration', 'fail_on_warning'],
)
if self._config['mkdocs']:
return Mkdocs(**self._config['mkdocs'])
return None
Expand All @@ -941,10 +882,6 @@ def doctype(self):

@property
def submodules(self):
Submodules = namedtuple( # noqa
'Submodules',
['include', 'exclude', 'recursive'],
)
return Submodules(**self._config['submodules'])


Expand Down
37 changes: 37 additions & 0 deletions readthedocs/config/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
"""Models for the response of the configuration object."""
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 can refactor these to be dataclasses when we are fully running py3 😎

Copy link
Member

Choose a reason for hiding this comment

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

You can create an issue for this and add it to the Refactor milestone.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done #4525


from __future__ import division, print_function, unicode_literals

from collections import namedtuple


Build = namedtuple('Build', ['image']) # noqa

Python = namedtuple( # noqa
'Python',
[
'version',
'requirements',
'install_with_pip',
'install_with_setup',
'extra_requirements',
'use_system_site_packages',
],
)

Conda = namedtuple('Conda', ['environment']) # noqa

Sphinx = namedtuple( # noqa
'Sphinx',
['builder', 'configuration', 'fail_on_warning'],
)

Mkdocs = namedtuple( # noqa
'Mkdocs',
['configuration', 'fail_on_warning'],
)

Submodules = namedtuple( # noqa
'Submodules',
['include', 'exclude', 'recursive'],
)
Loading