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

Update docs #3498

Merged
merged 19 commits into from
Feb 18, 2018
Merged

Update docs #3498

merged 19 commits into from
Feb 18, 2018

Conversation

@stsewd
Copy link
Member Author

stsewd commented Jan 9, 2018

The most relevant commits are:

Note: the whole docs needs to be re-wrapped to 80 chars, for now I only did this for the files I touched.
Note2: This should be updated too (probably another PR) https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/doc_builder/base.py#L89 (the text of the autogenerated index file and the logic when to generate this file)

@stsewd stsewd mentioned this pull request Jan 9, 2018
@ericholscher
Copy link
Member

I'm generally -1 on breaking docs at 80 characters. I'd much prefer to have them broken on semantic meaning (eg. periods or commas). This has a bit more background here: http://rhodesmill.org/brandon/2012/one-sentence-per-line/, but I feel pretty strongly against just doing a straight "80 char" break.

@ericholscher
Copy link
Member

I'd also ask that we not refactor content when we also change it in the PR. It makes it almost impossible to see what actually changed, so almost impossible to review.

@ericholscher
Copy link
Member

In general I love docs changes though! Thanks for doing these, and I'll try and figure out what exactly is going on, so I can review it.

@ericholscher
Copy link
Member

Also, we should definitely have a doc style guide, to answer questions like these :)

@RichardLitt
Copy link
Member

@stsewd Can you add the doc styleguide in this PR? It can probably go in the CONTRIBUTE doc on the website if it is small, at this point.

@RichardLitt RichardLitt added Needed: documentation Documentation is required PR: work in progress Pull request is not ready for full review labels Jan 11, 2018
@ericholscher
Copy link
Member

We can probably steal a decent bit from here: https://github.com/writethedocs/www/blob/master/docs/style-guide.rst

@stsewd
Copy link
Member Author

stsewd commented Jan 12, 2018

@RichardLitt I added some guidelines as suggested :). I feel like we need to improve a little more the section for contributing to the docs (and putted it a little closer to the Contributing to Read the Docs section), but probably another PR.

@ericholscher you are right, breaking the lines at semantic meaning rather that 80 chars is a good idea, the explanation from the post is very clear. Should I revert the other commits then?

docs/builds.rst Outdated
@@ -37,8 +37,6 @@ We will look inside a ``doc`` or ``docs`` directory first,
and then look within your entire project.

Then Sphinx will build any files with an ``.rst`` extension.
If you have a ``README.rst``,
it will be transformed into an ``index.rst`` automatically.

Mkdocs
Copy link
Member Author

Choose a reason for hiding this comment

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

Should be MkDocs? (uppercase D)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's the correct way of writting it: http://www.mkdocs.org/

@stsewd
Copy link
Member Author

stsewd commented Jan 24, 2018

I reverted the noisy commits and applied the style guide as mentioned, this is ready for another review.

@RichardLitt RichardLitt added PR: ready for review and removed PR: work in progress Pull request is not ready for full review labels Jan 25, 2018
This was referenced Feb 15, 2018
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.

Nice! Thanks @stsewd. I left a couple of comments for you to take a look :)

docs/docs.rst Outdated
@@ -10,4 +10,29 @@ You can build the docs by installing ``Sphinx`` and running::
# in the docs directory
make html

Let us know if you have any questions or want to contribute to the documentation.
Please follow these guidelines when updating our docs.
Let us know if you have any questions or if something isn't clear.
Copy link
Member

Choose a reason for hiding this comment

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

Remove the if from "if something"

docs/docs.rst Outdated
-------

* Do not break the content across multiple lines at 80 characters,
but rather broken them on semantic meaning (e.g. periods or commas).
Copy link
Member

Choose a reason for hiding this comment

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

break them

Copy link
Member

Choose a reason for hiding this comment

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

Also, I think it would be awesome to link the post that Eric shared with you. You said that was useful to understand this idea, so may be good to link it here.

@@ -57,7 +57,7 @@ environment, and will be set to ``True`` when building on RTD::
{% endif %}

I get import errors on libraries that depend on C modules
----------------------------------------------------------
---------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

Did you remove this for some reason? Just curious :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@humitos the header is larger than the text p:

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's OK. Github doesn't show it in a monospace font for some reason:

captura de pantalla_2018-02-16_09-18-14

Copy link
Member Author

Choose a reason for hiding this comment

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

I use rstcheck for linting rst files :)

Copy link
Member

Choose a reason for hiding this comment

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

Wow! I didn't know it and I'm a fan of @myint.

Considering that we are talking about the documentation style, I think it would be a good addition for travis checks and linting right? I mean, trigger a linting on the docs on travis checks.

Would you like to add an issue for this and take a look on what's needed and how to implement it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I opened a new issue #3622, I will implemented later :)

@stsewd
Copy link
Member Author

stsewd commented Feb 16, 2018

@humitos all the corrections were done, thanks!

@humitos humitos removed the Needed: documentation Documentation is required label Feb 16, 2018
@humitos
Copy link
Member

humitos commented Feb 16, 2018

Nice! I'm asking Eric for review this since he is the "English guy". Although, it's good enough for me.

@ericholscher ericholscher merged commit f725917 into readthedocs:master Feb 18, 2018
@ericholscher
Copy link
Member

Love doc updates :) 💯

@RichardLitt
Copy link
Member

@humitos Feel free to ask me for English updates, too. :)

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.

4 participants