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

Document that readthedocs file can now have yaml extension #4129

Merged
merged 4 commits into from
Jun 18, 2018
Merged

Document that readthedocs file can now have yaml extension #4129

merged 4 commits into from
Jun 18, 2018

Conversation

StefanoChiodino
Copy link
Contributor

Fixes #4102. Documents changes over readthedocs/readthedocs-build#48

@RichardLitt
Copy link
Member

Looks like there's a failing check on this. Can you see if you can fix that?

@StefanoChiodino
Copy link
Contributor Author

Yep, not sure if I'll manage today. Also there are other tests that broke on the build repo, but pass on my machine 😰

@@ -1,5 +1,5 @@
# -*- coding: utf-8 -*-
"""An API to load config from a readthedocs.yml file."""
"""An API to load config from a 'readthedocs.yml', 'readthedocs.yaml', or '.readthedocs.yml' file."""
Copy link
Member

Choose a reason for hiding this comment

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

I think this can remain as readthedocs.yml file, is kind of clear enough that the name can be .rea... etc. Just a thought.

@@ -134,7 +134,7 @@ def build_image(self):

def load_yaml_config(version):
"""
Load a configuration from `readthedocs.yml` file.
Load a configuration from `readthedocs.yml`, `readthedocs.yaml`, or `.readthedocs.yml` file.
Copy link
Member

Choose a reason for hiding this comment

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

Same here, and those lines are the origin of the linter error (>80 columns)

@RichardLitt
Copy link
Member

@StefanoChiodino No rush on that. :) Be at ease!

docs/conda.rst Outdated
@@ -15,7 +15,8 @@ Activating Conda
----------------

Conda Support is the first feature enabled with :doc:`yaml-config`.
You can enable it by creating a `readthedocs.yml` file in the root of your repository with the contents:
You can enable it by creating a `readthedocs.yml`, `readthedocs.yaml`, or `.readthedocs.yml` file in the root of your
Copy link
Member

Choose a reason for hiding this comment

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

I think the link the to the yaml docs are fine here and we can just simplify this by Read the Docs configuration file or just .readthedocs.yml. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Santos. We should mention just one possible option here and give more details below by listing all possible options as you have done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, should we go for readthedocs.yml here? Looks like the simplest option.

@stsewd
Copy link
Member

stsewd commented Jun 8, 2018

Can you please rebase/merge against master? We added a new linter for our docs the last week

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

I think we need to generalize the idea of the file names and give details in just one place about all the accepted names.

I'd happy to merge it after these changes are done. Thanks!

docs/conda.rst Outdated
@@ -15,7 +15,8 @@ Activating Conda
----------------

Conda Support is the first feature enabled with :doc:`yaml-config`.
You can enable it by creating a `readthedocs.yml` file in the root of your repository with the contents:
You can enable it by creating a `readthedocs.yml`, `readthedocs.yaml`, or `.readthedocs.yml` file in the root of your
Copy link
Member

Choose a reason for hiding this comment

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

I agree with Santos. We should mention just one possible option here and give more details below by listing all possible options as you have done.

- ``readthedocs.yml``
- ``readthedocs.yaml``
- ``.readthedocs.yml``
Copy link
Member

Choose a reason for hiding this comment

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

List all possible options here.

@@ -2,7 +2,7 @@ Read the Docs YAML Config
=========================

Read the Docs now has support for configuring builds with a YAML file.
The file, ``readthedocs.yml`` (or ``.readthedocs.yml``), must be in the root directory of your project.
The file, ``readthedocs.yml``, ``.readthedocs.yml``, or ``readthedocs.yaml``, must be in the root directory of your project.
Copy link
Member

Choose a reason for hiding this comment

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

Use only one file name here (the same than in the first paragraph)

@StefanoChiodino
Copy link
Contributor Author

@humitos / @stsewd are you happy with it now? Not sure how that check is failing. Seems to be stuck in travis?

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Thanks! I think this is good and cleaner.

We just refer to it with a single name but when giving details we show all the possibilities.

@humitos humitos merged commit f5e8705 into readthedocs:master Jun 18, 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
Development

Successfully merging this pull request may close these issues.

5 participants