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

Improve yaml config docs #3685

Merged
merged 7 commits into from May 22, 2018
Merged

Conversation

@stsewd
Copy link
Member

@stsewd 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 readthedocs/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
Copy link
Member Author

@stsewd stsewd Feb 26, 2018

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

Copy link
Member

@humitos humitos Feb 26, 2018

Do we have a test that make this example fail?

Copy link
Member

@humitos humitos Feb 26, 2018

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

Copy link
Member Author

@stsewd stsewd Feb 26, 2018

Now we have one :) readthedocs/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

Copy link
Member Author

@stsewd stsewd Feb 26, 2018

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

Copy link
Member Author

@stsewd stsewd Feb 26, 2018

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

Copy link
Contributor

@agjohnson agjohnson Mar 6, 2018

👍 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
Copy link
Member Author

@stsewd 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
Copy link
Member

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

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

Copy link
Contributor

@agjohnson agjohnson left a comment

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
Copy link
Contributor

@agjohnson agjohnson Mar 6, 2018

👍 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
Copy link
Member Author

@stsewd stsewd commented Mar 6, 2018

This is blocked due readthedocs/readthedocs-build#43

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented May 3, 2018

Unblocked!

@agjohnson agjohnson removed this from the Better user documentation milestone May 3, 2018
@agjohnson agjohnson added this to the 2.4 milestone May 3, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented May 7, 2018

This is done

Copy link
Member

@humitos humitos left a comment

Good work!

humitos
humitos approved these changes May 8, 2018
@ericholscher ericholscher merged commit 53ba3ca into readthedocs:master May 22, 2018
1 check passed
@stsewd stsewd deleted the 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants