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

Implement sphinx key from v2 config #4482

Merged
merged 21 commits into from Sep 7, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Aug 6, 2018

This PR on top of #4456. It should be merged after.

New commits are from 6756b7f

Ref #4219

@humitos
Copy link
Member

@humitos humitos commented Aug 15, 2018

I will wait until the base PR gets merged to review this.

Loading

@stsewd stsewd force-pushed the build-config-v2-phinx branch from 64cd38a to 58adc35 Aug 25, 2018
@@ -232,7 +232,6 @@ def test_requirements_file(self, load_config):
self.assertEqual(config.python.requirements, '__init__.py')


@pytest.mark.skip
Copy link
Member Author

@stsewd stsewd Aug 25, 2018

Choose a reason for hiding this comment

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

Tests are un-skipped here

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 25, 2018

Rebased and updated! I'll try to manually test this by morning monday

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 27, 2018

Ok, I need to figure out how to keep the previous behavior that creates a conf.py file if no one is found and use that.

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 27, 2018

I'll like to merge this first #4566, as I'm touching some related code and I want to make sure I'm not breaking that code.

Loading

def test_create_conf_py(
self, get_conf_py_path, _, get_config_params,
@patch('readthedocs.projects.models.Project.checkout_path')
def test_multiple_conf_py(
Copy link
Member Author

@stsewd stsewd Aug 27, 2018

Choose a reason for hiding this comment

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

Silly me, left this test with a duplicated name...

Loading

@@ -97,14 +114,24 @@ def test_create_conf_py(
"""

tmp_docs_dir = py.path.local(tempfile.mkdtemp())
tmp_docs_dir.join('conf.py').new()
tmp_docs_dir.join('test').mkdir().join('conf.py').new()
tmp_docs_dir.join('conf.py').write('')
Copy link
Member Author

@stsewd stsewd Aug 27, 2018

Choose a reason for hiding this comment

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

Looks like new() doesn't create the files on disk

Loading

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Aug 27, 2018

#4456 is merged, this needs a rebase.

Loading

@agjohnson agjohnson added this to the YAML File Completion milestone Aug 27, 2018
Copy link
Contributor

@agjohnson agjohnson left a comment

Looks good so far! Noted a couple places that would be helpful to ensure our logic isn't changing, or at least is not likely to introduce tricky bugs.

Loading

outfile_path = (
self.config.sphinx.configuration or
self.project.conf_file(self.version.slug)
)
Copy link
Contributor

@agjohnson agjohnson Aug 27, 2018

Choose a reason for hiding this comment

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

It seems here we're implying that if you set a sphinx configuration file, but it doesn't exist, we just create it? I feel like we should error out if you set up a config file and it doesn't actually exist, it will be confusing to the user for the build to pass in this case.

Loading

Copy link
Member Author

@stsewd stsewd Aug 27, 2018

Choose a reason for hiding this comment

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

This is to keep compatibility with the behavior when the user doesn't set a config file and rtd tries to find one.

Loading

Copy link
Contributor

@agjohnson agjohnson Aug 28, 2018

Choose a reason for hiding this comment

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

That part I understand, but this seemed to be creating a 3rd option, where the config was specified but not found, and therefore the config was generated instead. This might not be an issue, I interpreted self.config.sphinx.configuration as defaulting to None if the file is specified but doesn't exist, but that doesn't seem to be the logic used looking at it again.

Loading

config_file = (
self.config.sphinx.configuration or
self.project.conf_file(self.version.slug)
)
Copy link
Contributor

@agjohnson agjohnson Aug 27, 2018

Choose a reason for hiding this comment

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

Same here. And I feel like we shouldn't be guessing here. If the config file was specified and it exists, that's when we should set the config file path. Likewise, if we're generating a config, we set a config file path when we find out the user doesn't have a config and didn't specify a config file path.

This will cut down on magical decisions that tend to hide errors from us.

Loading

cwd = self.project.conf_dir(self.version.slug)
config_file = self.config.sphinx.configuration
config_dir = os.path.dirname(config_file) if config_file else None
cwd = config_dir or self.project.conf_dir(self.version.slug)
Copy link
Contributor

@agjohnson agjohnson Aug 27, 2018

Choose a reason for hiding this comment

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

This logic looks duplicated, is there a way to reduce this duplication if so?

Loading

Copy link
Member Author

@stsewd stsewd Aug 27, 2018

Choose a reason for hiding this comment

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

I knew you were not going to like it p: haha, so all this is to make sure that we are keeping the previous behavior. If a config file doesn't exist, we create one, and also we try to find one in the whole project. I guess we can do a big refactor to do this step early, before parsing the config file (but not sure if this worth it). Or maybe we can use a config_file property inside the sphinx builder and modify that as we want

Loading

Copy link
Contributor

@agjohnson agjohnson Aug 28, 2018

Choose a reason for hiding this comment

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

Yeah, this could either be a property, or even just simply set the common values on __init__ in base?

Loading

sphinx_configuration = path.join(
version.get_conf_py_path(),
'conf.py'
)
Copy link
Contributor

@agjohnson agjohnson Aug 27, 2018

Choose a reason for hiding this comment

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

It's not clear to me why this change is needed. Shouldn't the full path have already been returned by version.get_conf_py_path()?

Loading

Copy link
Member Author

@stsewd stsewd Aug 28, 2018

Choose a reason for hiding this comment

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

I was bitten by the name here too, this only returns the directory name. There is a test unskipped that covers this case.

Loading

Copy link
Contributor

@agjohnson agjohnson Aug 28, 2018

Choose a reason for hiding this comment

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

huh. is there a reason why this was working then? It seems like it shouldn't have worked at all if it was just the directory.

Loading

Copy link
Member Author

@stsewd stsewd Aug 28, 2018

Choose a reason for hiding this comment

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

Loading

Copy link
Member Author

@stsewd stsewd Aug 28, 2018

Choose a reason for hiding this comment

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

Also currently we are reading this from the database, that's why the builds don't fail

Loading

outfile = codecs.open(outfile_path, encoding='utf-8', mode='a')
self.config_file = (
self.config_file or
self.project.conf_file(self.version.slug)
Copy link
Member Author

@stsewd stsewd Aug 29, 2018

Choose a reason for hiding this comment

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

I need to keep this one, as here we catch and re-raise the exception to the user

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 29, 2018

V1 is still working, I refactored the code. I'll be testing v2 in the morning.

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 29, 2018

OK, v2 tested! It works :D

Loading

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Sep 7, 2018

Changes look good!

Loading

@agjohnson agjohnson merged commit 8705717 into readthedocs:master Sep 7, 2018
1 check passed
Loading
@stsewd stsewd deleted the build-config-v2-phinx branch Sep 7, 2018
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