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

Enforce striter CI checks for docs and fix some doc issues that would now fail the build. #1437

Merged
merged 3 commits into from
Sep 5, 2019

Conversation

cburgdorf
Copy link
Contributor

What was wrong?

The current CI job to validate the docs is to weak. In fact, there are some issues that went under the radar:

  1. One JSON code snippet isn't valid JSON which turns off the formatter
  2. One doc isn't included in the table of contents

How was it fixed?

  1. Make errors fail the build
  2. Include full tracebacks in errors
  3. Do not use cache to build the docs
  4. Add web3.tools.rst to the TOC
  5. Fix JSON formatting in example

Todo:

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@cburgdorf cburgdorf changed the title Christoph/docs/stricter checks Enforce striter CI checks for docs and fix some doc issues that would now fail the build. Aug 27, 2019
@cburgdorf
Copy link
Contributor Author

@kclowes would like to ping you for a review on this, too!

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Thanks @cburgdorf! I'm not sure that we want to include the web3.tools docs. I'm pretty sure they're autogenerated and might be more confusing than useful. Pinging @njgheorghita for an opinion, though. Otherwise this is awesome, thank you!

@cburgdorf
Copy link
Contributor Author

cburgdorf commented Aug 28, 2019

Ah, I see. Yes, it looks like the web3.tools.rst is auto-generated but I'm not sure from where. I'm not a sphinx pro and I haven't auto generated rst files before (we do auto read API docs in Trinity, but the RST files to pick and choose which API docs to read, we create from hand).

Anyway, this would need to be resolved to allow these stricter checks. With the stricter checks that this PR proposes (and which we use for Trinity, Py-EVM, Lahja) rst files that aren't included anywhere are treated as error. So either the automatic generation of this file needs do be prevented or the file needs to be included anywhere.

Feel free to ignore this PR if it doesn't fit into the current approach to docs for this repo. In generally, turning sphinx warnings into errors has worked very well for us so far as it forces you to fix things that are usually worth fixing (like the formatter ignoring that JSON snippet because it doesn't recognize it as valid JSON).

@kclowes
Copy link
Collaborator

kclowes commented Aug 28, 2019

I like the idea of making sphinx warnings into errors, because it seems like we are constantly dealing with new warnings after fixing them 😆 I'll take a look at how we can stop generating the web3.tools.rst file soon!

@kclowes kclowes force-pushed the christoph/docs/stricter-checks branch from 20c5f1c to 4b556e0 Compare September 5, 2019 18:32
@kclowes kclowes merged commit c1fa7e3 into ethereum:master Sep 5, 2019
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.

2 participants