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

have pytest run code examples in docs + have travis build docs #59

Merged
merged 3 commits into from
Jun 15, 2016

Conversation

jab
Copy link
Contributor

@jab jab commented Jun 12, 2016

Proof of concept. Anyone want to take it from here?
ref: #57

@jab jab mentioned this pull request Jun 12, 2016
@jab
Copy link
Contributor Author

jab commented Jun 12, 2016

Note, I expected Travis to fail here. I fixed the first several errors from running the code examples in the docs, but had to stop short of fixing all of them. Hopefully @dabeaz or another contributor can take it from here!

@@ -4,7 +4,8 @@ python: 3.5
sudo: false

install:
- python3 setup.py install
- travis_retry pip install -e .[test]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the use of travis_retry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw it in another project's .travis.yml (which seemed like it knew what it was doing), and it seemed like a zero-cost line of defense against e.g. transient network failures and other things that shouldn't cause the build to fail. Any reason not to?

Copy link
Contributor

Choose a reason for hiding this comment

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

I honestly don't know, hence the question. 😄

@brettcannon
Copy link
Contributor

I think for a first PR it should only worry about building the documentation and not conflate also making the examples all work with py.test as one doesn't necessitate the other.

Having said that, the build rule could also run linkcheck to look for stale links (found out about from here), with -n and -W to make sure errors are found (found out about here), and run with -q to make sure the doc output doesn't crowd out test results (found out about here).

@dabeaz
Copy link
Owner

dabeaz commented Jun 15, 2016

I'm not sure how easy it will be to make different code examples in the docs work under testing. All things equal, I'd probably want to start with something really simple first. For example, what Brett suggested where testing mainly just makes sure that the docs build. I'm not an expert at setting this up though so I'll have to defer to those with more experience.

- comment out running code examples from docs
- run linkcheck
- turn warnings into errors
- run with -q
@jab
Copy link
Contributor Author

jab commented Jun 15, 2016

Implemented the latest requested changes in 86ff1f7. Fixed a few of the resulting build errors. Then I got this one:

Warning, treated as error:
/Users/_pants/tmp/curio/docs/reference.rst:77: WARNING: py:class reference target not found: Task

I tried changing :class:'Task' to :class:'Task <curio.task.Task>'[1] and making sure curio.task exports Task in its __all__, but it's still failing with

Warning, treated as error:
/Users/_pants/tmp/curio/docs/reference.rst:77: WARNING: py:class reference target not found: curio.task.Task

Hopefully this is a good start though and you can take it from here. I've added you both as collaborators to my fork in case you want to play with this branch there, otherwise you could merge these changes to a branch here upstream and get it in the shape you want it to merge.

[1] Had to replace backticks with apostrophes to get GitHub to render properly

@brettcannon
Copy link
Contributor

I think the error you're seeing is because there's no definition for .. class:: Task definition in the docs.

To prevent this from being held up and so you get credit for the PR, @jab , why don't you remove the -W argument so the tests pass (we can open a separate issue about eliminating the warnings once this PR gets in) and back out the changes you made to make the py.test doc changes work? That way this PR is isolated to only being about updating .travis.yml and you get contribution recognition for all the work you have put in.

If this is too much of a hassle, though, just let me know and I can create a PR for all of this.

@jab
Copy link
Contributor Author

jab commented Jun 15, 2016

Removed -W in 5b5c144. Good call on the missing .. class:: Task. (Not sure if using Sphinx's autodoc would take care of populating any such missing definitions with at least some auto-generated documentation, but if so that might be nice to have at least in the meantime. Here's an example of using it in case it helps: source –> output)

@jab
Copy link
Contributor Author

jab commented Jun 15, 2016

Tests now pass.

@jab
Copy link
Contributor Author

jab commented Jun 15, 2016

Just to confirm, you want these changes backed out before merging? https://github.com/dabeaz/curio/pull/59/files#diff-1d39468fa376ebb3d0827a66affef99f

I can do that, but as far as I can tell they're not hurting anything, and someone will just have to do them again once the docs' code examples start getting tested.

tutorial
reference
howto
devel
Copy link
Contributor Author

@jab jab Jun 15, 2016

Choose a reason for hiding this comment

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

Btw, I moved these under the .. toctree:: here assuming that was the intention. Also added devel because otherwise sphinx generates a warning that it's not part of any toctree. The resulting built docs are the same, except the bulleted list in the "Contents" section of index.html is now generated automatically, including 2nd-level topics as per the :maxdepth: 2 (which I didn't change). Could change that to :maxdepth: 1 to make it condensed, as it was with the manually-generated bulleted list before.

@brettcannon
Copy link
Contributor

brettcannon commented Jun 15, 2016

Backing out the doc changes to make py.test pass are a call for @dabeaz , but my assumption is that unless you convert all the docs then doing it piecemeal might look a bit odd.

@dabeaz
Copy link
Owner

dabeaz commented Jun 15, 2016

I'm going to accept this, but I may revert the devel.rst docs back to my original style afterwards. Wasn't my intent for the docs to necessarily follow a doctest style.

@dabeaz dabeaz merged commit bf2cd7b into dabeaz:master Jun 15, 2016
@jab jab deleted the jab-fix-issue-57 branch June 15, 2016 19:04
@jab
Copy link
Contributor Author

jab commented Jun 15, 2016

Wasn't trying to cramp your style, just saw #57 (comment) and said "I know how to do that!" Just trying to be helpful.

@dabeaz
Copy link
Owner

dabeaz commented Jun 15, 2016

Yep. No problem! The 'devel.rst' doc is a big work in progress so a lot of stuff there is actively changing. Honestly, I hadn't thought about doctest too much considering how weird the interactive interpreter is for experimenting with coroutines.

@jab
Copy link
Contributor Author

jab commented Jun 15, 2016

Gotcha. Hope the PR ends up being helpful.

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.

None yet

3 participants