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

Projects
None yet
3 participants
@stsewd
Member

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

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 1, 2018

Member

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?

Member

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

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 1, 2018

Member

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

Member

stsewd commented Aug 1, 2018

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

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Aug 2, 2018

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 2, 2018

Member

@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.

Member

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 from Config file v2 implementation to Match v1 config interface to new one Aug 2, 2018

@@ -0,0 +1,37 @@
"""Models for the response of the configuration object."""

This comment has been minimized.

@stsewd

stsewd Aug 2, 2018

Member

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

@stsewd

stsewd Aug 2, 2018

Member

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

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

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

@humitos

humitos Aug 15, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Aug 15, 2018

Member

Done #4525

@stsewd
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 != '':

This comment has been minimized.

@stsewd

stsewd Aug 3, 2018

Member

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

@stsewd

stsewd Aug 3, 2018

Member

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

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

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

@humitos

humitos Aug 15, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Aug 15, 2018

Member

Adding the another solution in #4523

@stsewd

stsewd Aug 15, 2018

Member

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,

This comment has been minimized.

@stsewd

stsewd Aug 3, 2018

Member

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

@stsewd

stsewd Aug 3, 2018

Member

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

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 3, 2018

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 3, 2018

Member

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.

Member

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.

@humitos

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:

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

Did we remove this option?

@humitos

humitos Aug 15, 2018

Member

Did we remove this option?

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

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

@humitos

humitos Aug 15, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Aug 15, 2018

Member

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

@stsewd

stsewd Aug 15, 2018

Member

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'] = []

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

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

@humitos

humitos Aug 15, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Aug 15, 2018

Member

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

@stsewd

stsewd Aug 15, 2018

Member

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

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

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

I think this message is wrong. It will say

"python" section must be a mapping.

but we are under conda section.

@humitos

humitos Aug 15, 2018

Member

I think this message is wrong. It will say

"python" section must be a mapping.

but we are under conda section.

This comment has been minimized.

@stsewd

stsewd Aug 15, 2018

Member

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.

@stsewd

stsewd Aug 15, 2018

Member

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.

This comment has been minimized.

@humitos

humitos Aug 16, 2018

Member

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.

@humitos

humitos Aug 16, 2018

Member

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.

This comment has been minimized.

@stsewd

stsewd Aug 22, 2018

Member

I opened #4530 for this

@stsewd

stsewd Aug 22, 2018

Member

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

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

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

@humitos

humitos Aug 15, 2018

Member

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

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

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

@humitos

humitos Aug 15, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Aug 15, 2018

Member

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

@stsewd

stsewd Aug 15, 2018

Member

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

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

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

@humitos

humitos Aug 15, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Aug 15, 2018

Member

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

@stsewd

stsewd Aug 15, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Aug 15, 2018

Member

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

@stsewd

stsewd Aug 15, 2018

Member

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

This comment has been minimized.

@humitos

humitos Aug 16, 2018

Member

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.

@humitos

humitos Aug 16, 2018

Member

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.

This comment has been minimized.

@stsewd

stsewd Aug 16, 2018

Member

So, it should be use_system_packages or just system_packages?

@stsewd

stsewd Aug 16, 2018

Member

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

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

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

@humitos

humitos Aug 15, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Aug 15, 2018

Member

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

@stsewd

stsewd Aug 15, 2018

Member

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': ''}

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

Same note for ignore

@humitos

humitos Aug 15, 2018

Member

Same note for ignore

args, kwargs = run.call_args
assert 'setup.py' in args
assert 'install' in args

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

Why not use assert_called_with_args here? or assert_has_calls?

@humitos

humitos Aug 15, 2018

Member

Why not use assert_called_with_args here? or assert_has_calls?

This comment has been minimized.

@stsewd

stsewd Aug 15, 2018

Member

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

@stsewd

stsewd Aug 15, 2018

Member

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

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

Same

@humitos
assert 'setup.py' not in args
assert 'install' in args
assert '.[docs]' in args

This comment has been minimized.

@humitos

humitos Aug 15, 2018

Member

same

@humitos
@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 16, 2018

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 16, 2018

Member

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

Member

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

@stsewd stsewd referenced this pull request Aug 16, 2018

Closed

Change validation message #4530

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Aug 24, 2018

Contributor

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

Contributor

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 rtfd:master Aug 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment