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.

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

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Aug 1, 2018

This PR is based on #4355 and should be merged after that.

This will implement the actual logic of v2 and use the same nametuples for v1.

New changes are from ed1ac25

I'm using pytest flavor for the tests.

Closes #4220

@humitos
Copy link
Member

@humitos humitos commented Aug 1, 2018

Isn't it better to merge this PR into #4355 so, it's easier to see the changes. Then, after that PR is merged, we can change merging branch again. What do you think?

@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 1, 2018

@humitos yeah, but unfortunately the branch is in my fork, so I can't change the base branch here :/

@stsewd stsewd force-pushed the build-config-v2-implementation branch from 6fb74d6 to cef7c03 Aug 1, 2018
@agjohnson agjohnson added this to the YAML File Completion milestone Aug 2, 2018
@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Aug 2, 2018

I tried to reset the base, but i'm not sure it worked. Looks like a manual rebase is in order.

This sounds like you are going to implement all of these changes in one PR, which I would advise against. This is going to make another PR that takes a lot of effort to review. I'd rather we break this up into discrete chunks for each feature.

@stsewd stsewd force-pushed the build-config-v2-implementation branch from 2afc899 to 5b37760 Aug 2, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 2, 2018

@agjohnson I just did the rebase to change the base, yeah, I think I can revert the latest change and skip the new tests (those were very helpful while refactoring btw p:). The other changes are only related to changing the v1 interface to match the new one.

@stsewd stsewd changed the title Config file v2 implementation Match v1 config interface to new one Aug 2, 2018
@@ -0,0 +1,37 @@
"""Models for the response of the configuration object."""
Copy link
Member Author

@stsewd stsewd Aug 2, 2018

We can refactor these to be dataclasses when we are fully running py3 😎

Copy link
Member

@humitos humitos Aug 15, 2018

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

Copy link
Member Author

@stsewd stsewd Aug 15, 2018

Done #4525

requirements_file_path = self.config.requirements_file
if not requirements_file_path:
requirements_file_path = self.config.python.requirements
if not requirements_file_path and requirements_file_path != '':
Copy link
Member Author

@stsewd stsewd Aug 3, 2018

This is needed for the new behavior of the requirements file, anyway, this doesn't affect the current behavior of v1 (v1 config returns null here).

Copy link
Member

@humitos humitos Aug 15, 2018

I want to echo what I said regarding using explicit ignore for this.

Also, self.config.python.requirements could return IGNORE when the YAML contains '' (in case we won't change that for the explicit way).

Copy link
Member Author

@stsewd stsewd Aug 15, 2018

Adding the another solution in #4523

@@ -295,7 +296,8 @@ def install_user_requirements(self):
'--exists-action=w',
'--cache-dir',
self.project.pip_cache_path,
'-r{0}'.format(requirements_file_path),
'-r',
requirements_file_path,
Copy link
Member Author

@stsewd stsewd Aug 3, 2018

I split those to make it easy to test, nothing breaking anyway

@@ -466,18 +472,20 @@ def run_build(self, docker, record):
self.update_documentation_type()

python_env_cls = Virtualenv
if self.config.use_conda:
if self.config.conda:
Copy link
Member Author

@stsewd stsewd Aug 3, 2018

Conda is None now, if the user is using conda this is a dictionary

Copy link
Member

@humitos humitos Aug 15, 2018

Check against explicit None here.

if self.config.conda is not None:

@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 3, 2018

Looks like v1 is still working with this PR, I'm testing v2 a little too.

@stsewd stsewd requested a review from agjohnson Aug 3, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 3, 2018

And v2 is tested too! Just need to fix two minor things, but those are covered with the test that I'm skipping right now.

Copy link
Member

@humitos humitos left a comment

Good! I think the changes are good.

Although, I left some comments (and questions!) that I'd like to be considered and may imply other changes as well.

raw_python['setup_py_install'])

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

@humitos humitos Aug 15, 2018

Did we remove this option?

Copy link
Member

@humitos humitos Aug 15, 2018

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

Copy link
Member Author

@stsewd stsewd Aug 15, 2018

This option was never implemented in v1, we are considering implementing this in v2 #4354

validate_string(extra_name)
)
if not python['install_with_pip']:
python['extra_requirements'] = []
Copy link
Member

@humitos humitos Aug 15, 2018

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

Copy link
Member Author

@stsewd stsewd Aug 15, 2018

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

@@ -451,11 +444,14 @@ def validate_conda(self):
self.PYTHON_INVALID_MESSAGE,
Copy link
Member

@humitos humitos Aug 15, 2018

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

@stsewd stsewd Aug 15, 2018

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

@humitos humitos Aug 16, 2018

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

@stsewd stsewd Aug 22, 2018

I opened #4530 for this

"""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

@humitos humitos Aug 15, 2018

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

@humitos humitos Aug 15, 2018

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

Copy link
Member Author

@stsewd stsewd Aug 15, 2018

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

@@ -212,7 +227,7 @@ def test_use_system_site_packages_defaults_to_false():
build = get_build_config({'python': {}}, get_env_config())
build.validate()
# Default is False.
assert not build.use_system_site_packages
assert not build.python.use_system_site_packages
Copy link
Member

@humitos humitos Aug 15, 2018

What's the correct here? use_system_site_packages or system_packages?

It seems that we are using all the other names from V2 but this one only from V1. I think we should call it as in V2 (https://github.com/rtfd/readthedocs.org/pull/4451/files#diff-33bbde7fca852e881b51aa90f9f86b3fR83) build.python.system_packages

Copy link
Member Author

@stsewd stsewd Aug 15, 2018

use_system_site_packages is from the main interface. Yeah, system_packages is short

Copy link
Member Author

@stsewd stsewd Aug 15, 2018

But we are calling this as a property, so use_system_packages is more descriptive for me, what do you think?

Copy link
Member

@humitos humitos Aug 16, 2018

If the change it's just search and replace, I'd go for it. Why? Because I think that we can reflect the same names in the yaml file than in the Python interface.

It's easier to follow the mapping. That's all.

Copy link
Member Author

@stsewd stsewd Aug 16, 2018

So, it should be use_system_packages or just system_packages?

update_docs = self.get_update_docs_task()
assert update_docs.config.build.image == build_image

def test_custom_build_image(self, checkout_path, tmpdir):
Copy link
Member

@humitos humitos Aug 15, 2018

What would happen if they define a build image in the YAML and also there is a project.container_image?

Copy link
Member Author

@stsewd stsewd Aug 15, 2018

container_image takes precedence, there is a test for that in the config module, I didn't want to repeat all the tests here, just to make sure we are using the values

self.create_config_file(
tmpdir,
{
'python': {'requirements': ''}
Copy link
Member

@humitos humitos Aug 15, 2018

Same note for ignore

args, kwargs = run.call_args

assert 'setup.py' in args
assert 'install' in args
Copy link
Member

@humitos humitos Aug 15, 2018

Why not use assert_called_with_args here? or assert_has_calls?

Copy link
Member Author

@stsewd stsewd Aug 15, 2018

The command is very large ['/path/to/pip', 'install', '--cache'...]

args, kwargs = run.call_args

assert 'setup.py' not in args
assert 'install' in args
Copy link
Member

@humitos humitos Aug 15, 2018

Same


assert 'setup.py' not in args
assert 'install' in args
assert '.[docs]' in args
Copy link
Member

@humitos humitos Aug 15, 2018

same

@stsewd stsewd force-pushed the build-config-v2-implementation branch from 564b966 to 4baf899 Aug 15, 2018
@humitos
Copy link
Member

@humitos humitos commented Aug 16, 2018

It seems that you rebase or did a push --force or similar. I'd avoid doing this when the PR is under "Request changes" because it complicated to check the new changes after that.

This PR in particular is too big and it's complicated to follow and review over and over.

Anyway, I think we are good here.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 16, 2018

It seems that you rebase or did a push --force or similar.

Yeah, that was a mistake :/ instead of making a new commit I did an amend...

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Aug 24, 2018

I didn't see anything major, so we can probably just merge this and QA this next week before doing a release!

@agjohnson agjohnson merged commit b242e9d into readthedocs:master Aug 24, 2018
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants