-
Notifications
You must be signed in to change notification settings - Fork 32
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
Improves configuration page of documentation #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I love how it looks rendered! Thanks!
I'm requesting very small changes and then we can merge it.
docs/configuration.rst
Outdated
@@ -1,45 +1,66 @@ | |||
Configuration | |||
============= | |||
|
|||
The default settings should be enough for most of the cases. | |||
Although, if you have special requirements this section may help you tune up a little the extension to make it work as you need. | |||
The default settings cover the most common RTD use case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not mention RTD here since the extension should work without any change in other places as well. Although, if you think it's necessary to mention it (since it's tied to the following example), "RTD" should be "Read the Docs".
I'm not sold on one or the other, but I want to avoid people not using the extension because they think that it's Read the Docs only. I added this paragraph in the index trying to mitigate this:
This extension was originally developed to be used on Read the Docs but it can be used in other hosting services as well.
But you probably knows more than me about writing docs 😄 , so I'm open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I updated the language - let me know what you think. (I also added Description: and Notes: to the individual config value listings where appropriate, this makes the file a sort of template for future entries.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thanks!
I feel like Description:
and Notes:
could be redundant since each entry is the description of the config itself, but looks way better that what we had. So, I'm merging it and we can change this in the future if we want and find a better way to do it.
Potential fix for #39.
Please read the edits carefully - some of my changes I can confirm based on my experience with the extension, but someone who knows the code should double-check them.
Note that the docstrings are still in an rst page - if the goal is to pull docstrings from the code, this is only a partial solution.