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

Projects
None yet
3 participants
@stsewd
Member

stsewd commented Aug 6, 2018

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

New commits are from 6756b7f

Ref #4219

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 15, 2018

Member

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

Member

humitos commented Aug 15, 2018

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

@@ -232,7 +232,6 @@ def test_requirements_file(self, load_config):
self.assertEqual(config.python.requirements, '__init__.py')
@pytest.mark.skip

This comment has been minimized.

@stsewd

stsewd Aug 25, 2018

Member

Tests are un-skipped here

@stsewd

stsewd Aug 25, 2018

Member

Tests are un-skipped here

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 25, 2018

Member

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

Member

stsewd commented Aug 25, 2018

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

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 27, 2018

Member

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.

Member

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.

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 27, 2018

Member

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.

Member

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.

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(

This comment has been minimized.

@stsewd

stsewd Aug 27, 2018

Member

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

@stsewd

stsewd Aug 27, 2018

Member

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

@@ -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('')

This comment has been minimized.

@stsewd

stsewd Aug 27, 2018

Member

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

@stsewd

stsewd Aug 27, 2018

Member

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

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Aug 27, 2018

Contributor

#4456 is merged, this needs a rebase.

Contributor

agjohnson commented Aug 27, 2018

#4456 is merged, this needs a rebase.

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

@agjohnson

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.

Show outdated Hide outdated readthedocs/doc_builder/backends/sphinx.py Outdated
Show outdated Hide outdated readthedocs/doc_builder/backends/sphinx.py Outdated
Show outdated Hide outdated readthedocs/doc_builder/backends/sphinx.py Outdated
sphinx_configuration = path.join(
version.get_conf_py_path(),
'conf.py'
)

This comment has been minimized.

@agjohnson

agjohnson Aug 27, 2018

Contributor

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

@agjohnson

agjohnson Aug 27, 2018

Contributor

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

This comment has been minimized.

@stsewd

stsewd Aug 28, 2018

Member

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

@stsewd

stsewd Aug 28, 2018

Member

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

This comment has been minimized.

@agjohnson

agjohnson Aug 28, 2018

Contributor

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.

@agjohnson

agjohnson Aug 28, 2018

Contributor

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.

This comment has been minimized.

@stsewd
@stsewd

stsewd Aug 28, 2018

Member

This comment has been minimized.

@stsewd

stsewd Aug 28, 2018

Member

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

@stsewd

stsewd Aug 28, 2018

Member

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

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

This comment has been minimized.

@stsewd

stsewd Aug 29, 2018

Member

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

@stsewd

stsewd Aug 29, 2018

Member

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

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 29, 2018

Member

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

Member

stsewd commented Aug 29, 2018

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

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 29, 2018

Member

OK, v2 tested! It works :D

Member

stsewd commented Aug 29, 2018

OK, v2 tested! It works :D

@stsewd stsewd requested a review from agjohnson Sep 4, 2018

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Sep 7, 2018

Contributor

Changes look good!

Contributor

agjohnson commented Sep 7, 2018

Changes look good!

@agjohnson agjohnson merged commit 8705717 into rtfd:master Sep 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd: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