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

Improve yaml config docs #3685

Merged
merged 7 commits into from May 22, 2018

Conversation

Projects
None yet
5 participants
@stsewd
Member

stsewd commented Feb 26, 2018

The current types on the documentation are a mix of python and yaml. Related issue #2813

  • According to the spec null is used http://yaml.org/spec/1.2/spec.html#id2803362
  • Also there isn't a functional example of a yaml file. I wanted to add the yaml file from rtd, but there isn't one, yet, so this could be another PR :).

On this PR rtfd/readthedocs-build#38 you can check that with this values the project works

@@ -26,7 +42,7 @@ Choose ``none`` to build none of the formats.
# Don't build any extra formats
formats:
- none
- null

This comment has been minimized.

@stsewd

stsewd Feb 26, 2018

Member

I think this would no work with the current implementation

https://github.com/rtfd/readthedocs-build/blob/daecec8b74af1f55b0ab4c1dd53d9cf6aee20126/readthedocs_build/config/config.py#L399-L409

This should be:

formats: null

This comment has been minimized.

@humitos

humitos Feb 26, 2018

Member

Do we have a test that make this example fail?

This comment has been minimized.

@humitos

humitos Feb 26, 2018

Member

Won't this change affect current users using it in this way?

This comment has been minimized.

@stsewd

stsewd Feb 26, 2018

Member

Now we have one :) rtfd/readthedocs-build#43

Won't this change affect current users using it in this way?

The current description doesn't work for any user using it

This comment has been minimized.

@stsewd

stsewd Feb 26, 2018

Member

Mmm, now I see, this value is hardcoded as a string on the code. What should be done now?

This comment has been minimized.

@stsewd

stsewd Feb 26, 2018

Member

and now that I know the syntax for an empty list, would be better that this is represented as []

This comment has been minimized.

@agjohnson

agjohnson Mar 6, 2018

Contributor

👍 on using the formats: [] syntax. This example will produce something we don't want:

In [1]: yaml.load("foo:\n  - null")
Out[1]: {'foo': [None]}
@stsewd

This comment has been minimized.

Member

stsewd commented Feb 26, 2018

Also there is a default type [] for an empty list, should be changed to be actually empty?

https://github.com/rtfd/readthedocs.org/blob/master/docs/yaml-config.rst#pythonextra_requirements

@humitos

This comment has been minimized.

Member

humitos commented Feb 26, 2018

Also there is a default type [] for an empty list, should be changed to be actually empty?

I think the empty list is the correct default object for a field where a list is expected (instead of None).

@stsewd

This comment has been minimized.

Member

stsewd commented Feb 26, 2018

@humitos I was thinking something like this

python:
    extra_requirements:

But that is None, anyway... How to represent an empty list on yaml?

Edit: I found it, indeed is [] like python 😁

@agjohnson

Great additions! 👍

We'll also want to make sure the empty list example for formats is correct and maybe even ensure this test case (we probably already do? good to check anyways)

@@ -26,7 +42,7 @@ Choose ``none`` to build none of the formats.
# Don't build any extra formats
formats:
- none
- null

This comment has been minimized.

@agjohnson

agjohnson Mar 6, 2018

Contributor

👍 on using the formats: [] syntax. This example will produce something we don't want:

In [1]: yaml.load("foo:\n  - null")
Out[1]: {'foo': [None]}

@agjohnson agjohnson added this to the Better user documentation milestone Mar 6, 2018

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 6, 2018

This is blocked due rtfd/readthedocs-build#43

@agjohnson

This comment has been minimized.

Contributor

agjohnson commented May 3, 2018

Unblocked!

@stsewd

This comment has been minimized.

Member

stsewd commented May 7, 2018

This is done

@humitos

humitos approved these changes May 8, 2018

@ericholscher ericholscher merged commit 53ba3ca into rtfd:master May 22, 2018

1 check passed

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

@stsewd stsewd deleted the stsewd:improve-docs-yaml-config branch May 22, 2018

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