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

Merged
merged 16 commits into from Sep 27, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Aug 7, 2018

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

New changes are from 3f311ef

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

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Sep 7, 2018

Base PR is merged, care to rebase this?

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 7, 2018

I'm rebasing this now

Loading

@stsewd stsewd force-pushed the build-config-v2-mkdocs branch 2 times, most recently from 8ad9594 to fe5de39 Sep 10, 2018
@stsewd stsewd force-pushed the build-config-v2-mkdocs branch 2 times, most recently from 1cb4d95 to 7892ec3 Sep 10, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 10, 2018

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

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 10, 2018

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

Loading

Copy link
Member Author

@stsewd stsewd left a comment

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.

Loading

@@ -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()
Copy link
Member Author

@stsewd stsewd Sep 11, 2018

Choose a reason for hiding this comment

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

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

Loading


# Web tasks
@app.task(queue='web')
def sync_files(project_pk, version_pk, hostname=None, html=False,
def sync_files(project_pk, version_pk, config, hostname=None, html=False,
Copy link
Member Author

@stsewd stsewd Sep 11, 2018

Choose a reason for hiding this comment

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

We need the current doctype, so I'm passing it to all tasks

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 11, 2018

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

Loading

@@ -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')
Copy link
Contributor

@davidfischer davidfischer Sep 11, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member Author

@stsewd stsewd Sep 11, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member

@humitos humitos Oct 1, 2018

Choose a reason for hiding this comment

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

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

Loading

@stsewd
Copy link
Member Author

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

Loading

@stsewd
Copy link
Member Author

@stsewd 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)

Loading

@stsewd
Copy link
Member Author

@stsewd 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?).

Loading

@stsewd stsewd self-assigned this Sep 12, 2018
@stsewd stsewd force-pushed the build-config-v2-mkdocs branch from a8db61a to 463f879 Sep 13, 2018
@stsewd stsewd force-pushed the build-config-v2-mkdocs branch 2 times, most recently from 9ddbb78 to ff1a8df Sep 17, 2018
@@ -236,7 +236,7 @@ def install_core_requirements(self):
'recommonmark==0.4.0',
]

if self.project.documentation_type == 'mkdocs':
if self.config.doctype == 'mkdocs':
Copy link
Member Author

@stsewd stsewd Sep 17, 2018

Choose a reason for hiding this comment

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

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.

Loading

@stsewd stsewd force-pushed the build-config-v2-mkdocs branch from 80e3904 to b0d23a6 Sep 18, 2018
@@ -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),
Copy link
Member Author

@stsewd stsewd Sep 18, 2018

Choose a reason for hiding this comment

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

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

Loading

@@ -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()
Copy link
Member Author

@stsewd stsewd Sep 18, 2018

Choose a reason for hiding this comment

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

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

Loading

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

Copy link
Member Author

@stsewd stsewd Sep 18, 2018

Choose a reason for hiding this comment

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

Removing these is a first step to deprecate Project.doctype

Loading

Copy link
Contributor

@agjohnson agjohnson left a comment

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

Loading

self.error(
key,
'Your documentation type should match '
'the one from the admin panel of your project.',
Copy link
Contributor

@agjohnson agjohnson Sep 19, 2018

Choose a reason for hiding this comment

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

nitpick: "project admin dashboard" instead of "admin panel of your panel"

Loading

Copy link
Contributor

@agjohnson agjohnson Sep 19, 2018

Choose a reason for hiding this comment

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

Also, perhaps instead mention the expected value here?

"Your project is configured as '{ project.documentation_type }' in your project admin dashboard, but there was no "sphinx" key specified in the configuration."

Something like that

Loading

@@ -1333,6 +1336,7 @@ def test_sphinx_builder_default(self):
build.validate()
build.sphinx.builder == 'sphinx'

@pytest.mark.skip
Copy link
Contributor

@agjohnson agjohnson Sep 19, 2018

Choose a reason for hiding this comment

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

Why is this skipped?

Perhaps we should get in the habit of code commenting why a test is xfail, skip, etc. Or i think you can do:

@pytest.mark.skip("Skipped because ...")

Loading

Copy link
Member Author

@stsewd stsewd Sep 19, 2018

Choose a reason for hiding this comment

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

Those tests are incompatible with the new behavior of checking the same doctype, all those changes are part of #4638 (comment)

Loading

self.project.save()

update_docs = self.get_update_docs_task()
update_docs.build_docs_html()

get_builder_class.assert_called_with(result)

@pytest.mark.skip
Copy link
Contributor

@agjohnson agjohnson Sep 19, 2018

Choose a reason for hiding this comment

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

Another spot to note why we're skipping

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 19, 2018

@agjohnson done!

Loading

Copy link
Contributor

@agjohnson agjohnson left a comment

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

Loading

'Your documentation type should match '
'the one from the admin panel of your project.',
'Your project is configured as "{doctype}" in your admin '
'dashboard, but there was a "{key}" key specified.'.format(
Copy link
Contributor

@agjohnson agjohnson Sep 20, 2018

Choose a reason for hiding this comment

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

One last nitpick. The error is sort of inverted i feel -- we don't actually tell the user the they need a "sphinx" key, just that they can't have a "mkdocs" key. I feel we need to be clearer here, ie:

Your project is configured as "Sphinx" in your project admin dashboard, but there is no "sphinx" key specified.

Also, for some of the odd documentation types, it looks like we're putting the slug in there, so:

Your project is configured as "sphinx_htmldir" ...

I feel like we want the verbose name instead. This shouldn't be a huge change, just using the model field to resolve the verbose name, but if it is a large change, it's not horribly important. This piece is temporary and it's probably close enough for most users.

Loading

Copy link
Member Author

@stsewd stsewd Sep 20, 2018

Choose a reason for hiding this comment

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

Mmm, I'm not sure about accessing the db from the config module (we don't do that there). I'm changing the error message.

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 20, 2018

@agjohnson done

Loading

Copy link
Contributor

@agjohnson agjohnson left a comment

Looks like the last fix wasn't correct.

Loading

)
self.error(
key,
'Your project is configured as "{doctype}" in your admin '
Copy link
Contributor

@agjohnson agjohnson Sep 20, 2018

Choose a reason for hiding this comment

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

doctype here is the interpolation that could be displayed as sphinx_htmldir. key in this string can only be sphinx or mkdocs.

Loading

'Your project is configured as "{doctype}" in your admin '
'dashboard, but there is no "{key}" key specified.'.format(
doctype=dashboard_doctype,
key=needed_doctype,
Copy link
Contributor

@agjohnson agjohnson Sep 20, 2018

Choose a reason for hiding this comment

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

I think what you meant was:

    doctype=needed_doctype,
    key=key,

Loading

Copy link
Member Author

@stsewd stsewd Sep 20, 2018

Choose a reason for hiding this comment

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

This was on purpose, as we say that the project is configured as sphinx_htmldir in the admin dashboard, and the user is missing a sphinx key in their config file.

key represents the current doctype from the config file.

Dashboard -> sphinx
Config -> mkdocs

Error: Your project is configured as "sphinx" in your admin dashboard, but there is no "sphinx" key specified.

The key is used to indicate where is the error something like File, key: error message

Loading

Copy link
Member Author

@stsewd stsewd Sep 20, 2018

Choose a reason for hiding this comment

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

so, in the previous example, key would be mkdocs We can put the key as . if that's looks confusing

Loading

Copy link
Member Author

@stsewd stsewd Sep 20, 2018

Choose a reason for hiding this comment

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

Also, we need the dashboard_doctype, otherwise we will have some weird errors like:

Dashboard -> sphinx_htmldir
Config -> sphinx.html

Your project is configured as "sphinx" in your admin dashboard, but there is no "sphinx" key specified

The current message for this case looks like:

Your project is configured as "sphinx_htmldir" in your admin dashboard, but there is no "sphinx" key specified

Isn't perfect neither... but better than the above p:

Loading

Copy link
Member Author

@stsewd stsewd Sep 20, 2018

Choose a reason for hiding this comment

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

What about Your project is configured as "sphinx_htmldir" in your admin dashboard, it doesn't match the documentation type from the configuration

Loading

Copy link
Contributor

@agjohnson agjohnson Sep 21, 2018

Choose a reason for hiding this comment

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

Isn't perfect neither... but better than the above p:

Exposing our internal slug to users isn't great UX, so i actually prefer the first. I wanted to originally use the verbose option name, but this is maybe annoying to get from the choice list -- for example, if i was using the model, i'd use the get_documentation_type_display() automatic function.

The _get_FIELD_display method is very small, we can likely replicate it very easily:
https://github.com/django/django/blob/cc79c7ee637e65c8da27e56d746c87903d5ec901/django/db/models/base.py#L902-L904

That way, we give the user an error that matches what the user sees in our admin, not what we see in our database. So, ideally:

Your project is configured as Sphinx HTMLdir in your admin dashboard,
but there is no "sphinx" key specified

Loading

'sphinx': 'Sphinx Html',
'sphinx_htmldir': 'Sphinx HtmlDir',
'sphinx_singlehtml': 'Sphinx Single Page HTML',
}
Copy link
Member Author

@stsewd stsewd Sep 21, 2018

Choose a reason for hiding this comment

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

I didn't want to bring these from the db, as the config module is isolated from the other rtd code

Loading

Copy link
Contributor

@agjohnson agjohnson Sep 25, 2018

Choose a reason for hiding this comment

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

I'm probably -1 on maintaining this in the config module separately. Was this to ensure we could move the config module out of rtd core?

I'd say either rely on the model field choices to report messages like ".. your project is configured as Sphinx HTMLdir", or skip all mentions of the Sphinx builder. I think what I was trying to describe in an earlier review was that we should skip mentioning the exact builder entirely if this complicates things too much (which seems like it is).

Loading

Copy link
Member Author

@stsewd stsewd Sep 25, 2018

Choose a reason for hiding this comment

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

Yeah, that was the reason. I think we can pair haha

Loading

Copy link
Contributor

@agjohnson agjohnson left a comment

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!

Loading

'sphinx': 'Sphinx Html',
'sphinx_htmldir': 'Sphinx HtmlDir',
'sphinx_singlehtml': 'Sphinx Single Page HTML',
}
Copy link
Contributor

@agjohnson agjohnson Sep 25, 2018

Choose a reason for hiding this comment

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

I'm probably -1 on maintaining this in the config module separately. Was this to ensure we could move the config module out of rtd core?

I'd say either rely on the model field choices to report messages like ".. your project is configured as Sphinx HTMLdir", or skip all mentions of the Sphinx builder. I think what I was trying to describe in an earlier review was that we should skip mentioning the exact builder entirely if this complicates things too much (which seems like it is).

Loading

code=INVALID_KEYS_COMBINATION,
)
error_msg = (
'Your project is configured as "{}" in your admin dashboard.'
Copy link
Contributor

@agjohnson agjohnson Sep 25, 2018

Choose a reason for hiding this comment

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

The error message shouldn't end with a period, it results in two sentence fragments:

Your project is configured as "Mkdocs" in your admin dashboard. But there is no "mkdocs" key specified.

It would be better as:

... in your admin, but there is no ..

Loading

'mkdocs' if dashboard_doctype == 'mkdocs' else 'sphinx'
)
else:
error_msg += ' But your "sphinx.builder" key does not match.'
Copy link
Contributor

@agjohnson agjohnson Sep 25, 2018

Choose a reason for hiding this comment

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

We're no longer catching the case where the user has no sphinx key at all. I still think this needs to be a separate check on top of checking if there is a sphinx key.

Loading

Copy link
Member Author

@stsewd stsewd Sep 25, 2018

Choose a reason for hiding this comment

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

The check for no sphinx key is above

Loading

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Sep 27, 2018

Thanks for repeatedly taking my nitpick abuse! 🙂

Looks good!

Loading

@agjohnson agjohnson merged commit df715de into readthedocs:master Sep 27, 2018
1 check passed
Loading
@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 27, 2018

No worries, I'm a nitpick person too haha

Loading

@stsewd stsewd deleted the build-config-v2-mkdocs branch Sep 27, 2018
@humitos
Copy link
Member

@humitos 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?

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 1, 2018

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

Loading

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

4 participants