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 mkdocs key from v2 config #4486

Merged
merged 16 commits into from Sep 27, 2018

Conversation

Projects
None yet
4 participants
@stsewd
Member

stsewd commented Aug 7, 2018

This is on top of #4482, it should be merged after.

New changes are from 3f311ef

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.

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Sep 7, 2018

Contributor

Base PR is merged, care to rebase this?

Contributor

agjohnson commented Sep 7, 2018

Base PR is merged, care to rebase this?

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Sep 7, 2018

Member

I'm rebasing this now

Member

stsewd commented Sep 7, 2018

I'm rebasing this now

@stsewd stsewd removed the Status: blocked label Sep 10, 2018

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Sep 10, 2018

Member

I had to use an empty commit, travis wasn't being executed, even forcing the push :/

Member

stsewd commented Sep 10, 2018

I had to use an empty commit, travis wasn't being executed, even forcing the push :/

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Sep 10, 2018

Member

I'll be testing this with a couple of mkdocs projects and see how it goes

Member

stsewd commented Sep 10, 2018

I'll be testing this with a couple of mkdocs projects and see how it goes

@stsewd

So, I realize that we use the doctype from the api and resolver, we don't have a config object there. We need to figure out how to pass the doctype there.

  • We can save the config as metadata in each version
  • We can save only the doctype in each version
  • We can write some values to a file as we do to detect an old environment.
@@ -470,10 +470,6 @@ def run_build(self, docker, record):
# Environment used for building code, usually with Docker
with self.build_env:
if self.project.documentation_type == 'auto':
self.update_documentation_type()

This comment has been minimized.

@stsewd

stsewd Sep 11, 2018

Member

I move this logic to the save method of the project model

@stsewd

stsewd Sep 11, 2018

Member

I move this logic to the save method of the project model

Show outdated Hide outdated readthedocs/projects/tasks.py Outdated
@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Sep 11, 2018

Member

Search tests are failing, but before fixing them, I'd like to resolve #4486 (review)

Member

stsewd commented Sep 11, 2018

Search tests are failing, but before fixing them, I'd like to resolve #4486 (review)

@@ -198,6 +199,8 @@ def build(self):
'--site-dir', self.build_dir,
'--config-file', self.yaml_file,
]
if self.config.mkdocs.fail_on_warning:
build_command.append('--strict')

This comment has been minimized.

@davidfischer

davidfischer Sep 11, 2018

Contributor

Personally, I'd let users configure this in their mkdocs.yml rather than adding an extra Read the Docs option.

@davidfischer

davidfischer Sep 11, 2018

Contributor

Personally, I'd let users configure this in their mkdocs.yml rather than adding an extra Read the Docs option.

This comment has been minimized.

@stsewd

stsewd Sep 11, 2018

Member

I'm +1 for this, I didn't know about that, Sphinx doesn't have something like that (I mean, using the conf.py file).

@stsewd

stsewd Sep 11, 2018

Member

I'm +1 for this, I didn't know about that, Sphinx doesn't have something like that (I mean, using the conf.py file).

This comment has been minimized.

@humitos

humitos Oct 1, 2018

Member

I think you could define setup() function in conf.py and override config.warningiserror = True and I think it should work.

@humitos

humitos Oct 1, 2018

Member

I think you could define setup() function in conf.py and override config.warningiserror = True and I think it should work.

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Sep 11, 2018

Member

We can use https://docs.djangoproject.com/en/1.9/ref/contrib/postgres/fields/#jsonfield to store metadata on the build/version object.

Member

stsewd commented Sep 11, 2018

We can use https://docs.djangoproject.com/en/1.9/ref/contrib/postgres/fields/#jsonfield to store metadata on the build/version object.

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Sep 11, 2018

Member

We can't use the readthedocs-env.json file, because it is saved on the builders (those are deleted frequently)

Member

stsewd commented Sep 11, 2018

We can't use the readthedocs-env.json file, because it is saved on the builders (those are deleted frequently)

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Sep 11, 2018

Member

So, what I think:

  1. use a json type on the build object (we can get this from version object, querying the latest build of that version).
  2. Or we can just add a new field (documentation_type) to the Version model.

I prefer 1), as we can save all the metadata used to build the docs, and query that data (like show the exact configuration used to build the docs). One downside is that we need to figure out how to make it work on development with sqlite.

Edit: We can use https://pypi.org/project/jsonfield/ or https://pypi.org/project/django-jsonfield/ if we want to keep the code database agnostic. Or maybe recommend using docker to new contributors (time to setup a docker-compose environment for dev?).

Member

stsewd commented Sep 11, 2018

So, what I think:

  1. use a json type on the build object (we can get this from version object, querying the latest build of that version).
  2. Or we can just add a new field (documentation_type) to the Version model.

I prefer 1), as we can save all the metadata used to build the docs, and query that data (like show the exact configuration used to build the docs). One downside is that we need to figure out how to make it work on development with sqlite.

Edit: We can use https://pypi.org/project/jsonfield/ or https://pypi.org/project/django-jsonfield/ if we want to keep the code database agnostic. Or maybe recommend using docker to new contributors (time to setup a docker-compose environment for dev?).

@stsewd stsewd self-assigned this Sep 12, 2018

@agjohnson agjohnson referenced this pull request Sep 17, 2018

Open

Deprecate `Project.documentation_type` #4638

0 of 6 tasks complete
@@ -236,7 +236,7 @@ def install_core_requirements(self):
'recommonmark==0.4.0',
]
if self.project.documentation_type == 'mkdocs':
if self.config.doctype == 'mkdocs':

This comment has been minimized.

@stsewd

stsewd Sep 17, 2018

Member

I'm leaving the doctype access in some places where we always use the config object, it doesn't break anything anyway. Just keeping the same pattern.

@stsewd

stsewd Sep 17, 2018

Member

I'm leaving the doctype access in some places where we always use the config object, it doesn't break anything anyway. Just keeping the same pattern.

@@ -138,7 +138,7 @@ def __init__(self, env_config, raw_config, source_file, source_position):
def error(self, key, message, code):
"""Raise an error related to ``key``."""
source = '{file} [{pos}]'.format(
file=self.source_file,
file=os.path.relpath(self.source_file, self.base_path),

This comment has been minimized.

@stsewd

stsewd Sep 18, 2018

Member

We were showing the full path here (/rtd/builds/...)

@stsewd

stsewd Sep 18, 2018

Member

We were showing the full path here (/rtd/builds/...)

@@ -566,6 +573,8 @@ def validate(self):
self.validate_doc_types()
self._config['mkdocs'] = self.validate_mkdocs()
self._config['sphinx'] = self.validate_sphinx()
# TODO: remove later
self.validate_final_doc_type()

This comment has been minimized.

@stsewd

stsewd Sep 18, 2018

Member

We need to remove this later #4638 (there is a note with the commit there)

@stsewd

stsewd Sep 18, 2018

Member

We need to remove this later #4638 (there is a note with the commit there)

def is_type_mkdocs(self):
"""Is project type Mkdocs."""
return 'mkdocs' in self.documentation_type

This comment has been minimized.

@stsewd

stsewd Sep 18, 2018

Member

Removing these is a first step to deprecate Project.doctype

@stsewd

stsewd Sep 18, 2018

Member

Removing these is a first step to deprecate Project.doctype

@agjohnson

Noted a couple small changes, but i think this looks good otherwise!

Show outdated Hide outdated readthedocs/config/config.py Outdated
Show outdated Hide outdated readthedocs/config/tests/test_config.py Outdated
Show outdated Hide outdated readthedocs/rtd_tests/tests/test_config_integration.py Outdated
@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd
Member

stsewd commented Sep 19, 2018

@agjohnson done!

@agjohnson

One last update on the error copy and I think we're good to merge.

Show outdated Hide outdated readthedocs/config/config.py Outdated
@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd
Member

stsewd commented Sep 20, 2018

@agjohnson done

@agjohnson

Looks like the last fix wasn't correct.

Show outdated Hide outdated readthedocs/config/config.py Outdated
Show outdated Hide outdated readthedocs/config/config.py Outdated
Show outdated Hide outdated readthedocs/config/config.py Outdated
@agjohnson

Sorry! More review on the error reporting. I think I might have suggested changes that complicate this PR further. If it helps to pair on this really quick, let me know!

Show outdated Hide outdated readthedocs/config/config.py Outdated
Show outdated Hide outdated readthedocs/config/config.py Outdated
Show outdated Hide outdated readthedocs/config/config.py Outdated
@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Sep 27, 2018

Contributor

Thanks for repeatedly taking my nitpick abuse! 🙂

Looks good!

Contributor

agjohnson commented Sep 27, 2018

Thanks for repeatedly taking my nitpick abuse! 🙂

Looks good!

@agjohnson agjohnson merged commit df715de into rtfd:master Sep 27, 2018

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Sep 27, 2018

Member

No worries, I'm a nitpick person too haha

Member

stsewd commented Sep 27, 2018

No worries, I'm a nitpick person too haha

@stsewd stsewd deleted the stsewd:build-config-v2-mkdocs branch Sep 27, 2018

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Oct 1, 2018

Member

I miss something here. I understood that you said that we were returning documentation_type in the API response. What are we returning now? If I'm reading correctly, we didn't remove that field from our DB and we are returning it as usual, but just added a validation on the config file that fails when the doctype from the YAML object differs from the one set in the admin. Am I right?

Member

humitos commented Oct 1, 2018

I miss something here. I understood that you said that we were returning documentation_type in the API response. What are we returning now? If I'm reading correctly, we didn't remove that field from our DB and we are returning it as usual, but just added a validation on the config file that fails when the doctype from the YAML object differs from the one set in the admin. Am I right?

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Oct 1, 2018

Member

@humitos yes, we keep the documentation_type here, we opened #4638 to track the complete remove

Member

stsewd commented Oct 1, 2018

@humitos yes, we keep the documentation_type here, we opened #4638 to track the complete remove

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment