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

Sphinx docs #39

Merged
merged 5 commits into from
Feb 8, 2017
Merged

Sphinx docs #39

merged 5 commits into from
Feb 8, 2017

Conversation

goodboy
Copy link
Contributor

@goodboy goodboy commented Jan 31, 2017

@hpk42 @RonnyPfannschmidt @nicoddemus so after much delay (and many iterations) I finally got around to finishing this. It's only an initial draft but I tried to cover as much as possible in an effort to address #14.

I think it's worth discussing some of the "internal" objects (those types prefixed by _) which should really just be public (although maybe not exposed for import via __all__).

Also note the current docs build is failing.

Let me know what you guys think as I'm happy to change anything you think could be better.

Thanks!

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

First let me thank you @tgoodlet for the excellent work! The documentation so far looks great and is an excellent start.

I left some small comments and suggestions, but please note that I'm not an English speaker so I might be off track on some of them.

docs/calling.rst Outdated
------------------
By default calling a hook results in all underlying :ref:`hookimpls
<impls>` functions to be invoked in sequence via a loop. Any function
which returns a none ``None`` result will have that result appended to
Copy link
Member

Choose a reason for hiding this comment

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

"none None" reads a little weird to me. Perhaps "returns a value different than None will have..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted I'll add it.

docs/calling.rst Outdated
a :py:class:`list` which is returned by the call.

The only exception to this behaviour is if the hook has been marked to return
its :ref:`firstresult` in which case only the first single none ``None`` value
Copy link
Member

Choose a reason for hiding this comment

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

Ditto about "none None" here.

docs/calling.rst Outdated

Historic calls
--------------
You can invoke certain hooks with all call *history* provided to each *hookimpl*
Copy link
Member

Choose a reason for hiding this comment

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

This sentence reads strange to me: "certain hooks with all call history provided to each hookimpl", IMO I think just removing this phrase and starting with "A historic call allows for all newly..." from the next paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good :)

docs/index.rst Outdated
Contents:
In fact, ``pytest`` is itself composed as a set of ``pluggy`` plugins
which are invoked in sequence according to a well defined set of protocols.
Some 150+ plugins use ``pluggy`` to extend and customize ``pytest``'s default behaviour.
Copy link
Member

Choose a reason for hiding this comment

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

Just realized some of these days that this number is over 273 now: http://plugincompat.herokuapp.com/

😁

Copy link
Contributor Author

@goodboy goodboy Feb 1, 2017

Choose a reason for hiding this comment

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

Ok 200+ it is!
I'll link to that page as well?

docs/index.rst Outdated

In essence, ``pluggy`` enables function `hooking`_ so you can build "pluggable" systems.
Copy link
Member

Choose a reason for hiding this comment

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

We might want to add a note that tox has been using pluggy as well to emphasize that pluggy is a general system.

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 was thinking about a section on Projects which use pluggy but I didn't know the full list offhand.

@nicoddemus is there any more besides tox and pytest plugins? execnet maybe?

subscriptions and can be thought of and used as a rudimentary busless `publish-subscribe`_
event system.

``pluggy``'s approach is meant to let a designer think carefuly about which objects are
Copy link
Member

Choose a reason for hiding this comment

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

Very nice!

docs/index.rst Outdated
Development
-----------
Great care must taken when hacking on ``pluggy`` since many mature
projects rely on it. Until there is a more automated CI process in
Copy link
Member

Choose a reason for hiding this comment

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

I think this part about "automated CI process" can be removed since we have AppVeyor and Travis automated testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh good point. Yeah I kind of forgot that there was already CI hehe.

@@ -1,23 +1,108 @@
.. pluggy documentation master file, created by
Copy link
Member

Choose a reason for hiding this comment

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

IMHO our index should only contain an index that points to other documentation... it helps to structure the documentation better. I suggest an index which looks (loosely) like:

  • Introduction
  • Specs/implementations
  • First example
  • Managing
  • Plugin Registry

Would be nice.

What do others think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicoddemus sure if you'd like?

Do you mean more like pytest's index.rst and docs homepage which seems to double as the github readme?

So you want me to strip the User Guide section (with its TOC) and have a docs section which links to individual .rst documents?

docs/define.rst Outdated
*options* passed as keyword arguments.


.. note::
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

docs/manage.rst Outdated
- :py:meth:`~pluggy.PluginManager.get_canonical_name()`- get a *plugin*'s
canonical name (the name it was registered with)
- :py:meth:`~pluggy.PluginManager.get_plugin()` - retrieve a plugin by its
canonical name using
Copy link
Member

Choose a reason for hiding this comment

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

This sentence seems incomplete

@goodboy
Copy link
Contributor Author

goodboy commented Feb 1, 2017

@nicoddemus fixed everything except for

  • the index.rst discussion
  • the missing list of major dependent projects

OH and I'll swash all those changes ^ into a single commit before we merge yeah?

@nicoddemus
Copy link
Member

the index.rst discussion

Yes let's wait for the opinions of others first.

into a single commit before we merge yeah?

Sounds good.

docs/define.rst Outdated
First result only
^^^^^^^^^^^^^^^^^
A *hookspec* can be marked such that when the *hook* is called the call loop
will only invoke up to the first *hookimpl* which returns a none ``None`` result.
Copy link
Member

Choose a reason for hiding this comment

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

Oops found another "none none" occurrence, for consistency we probably need to rephrase this as well.

@nicoddemus
Copy link
Member

Thanks @tgoodlet for the quick edits! 👍

@hpk42
Copy link
Contributor

hpk42 commented Feb 1, 2017

thanks @tgoodlet -- very good work and looking forward to see this online!
I'd prefer if we had fewer docs, though. I find editing and keeping a few documents consistent easier than many. We could then use automatic toc/content generation at the top of a single doc.

That being said, i am also fine to merge as is if you don't agree or don't want to go for it right now. Please be prepared then that i might do a PR sometime which reduces the number of docs, though :)

@goodboy
Copy link
Contributor Author

goodboy commented Feb 1, 2017

I find editing and keeping a few documents consistent easier than many. We could then use automatic toc/content generation at the top of a single doc.

So would you like the individual .rst files merged into one or two then @hpk42?

I guess I was just trying to follow the normal doc layout I've seen in other projects; pytest has its docs organized this way with multiple documents as well. I'm not sure what you mean by the automatic toc/content generation? If you can point me to how to use that in the sphinx docs I'll definitely take a look :)

Please be prepared then that i might do a PR sometime which reduces the number of docs, though :)

Heh of course that's always welcome :D

@hpk42
Copy link
Contributor

hpk42 commented Feb 2, 2017 via email

@goodboy
Copy link
Contributor Author

goodboy commented Feb 3, 2017

@hpk42 sounds good I'll make the change this weekend :)

@goodboy
Copy link
Contributor Author

goodboy commented Feb 3, 2017

Oh and just to remind myself I need to fix up the README.rst; it's still got come markdown links in it.

@goodboy
Copy link
Contributor Author

goodboy commented Feb 4, 2017

@hpk42 ok so after looking at what .. contents:: does I don't think it's necessary given that the sphinx theme we're using (which is also now the default by the way) already builds a TOC on the left hand side (you should try building from my branch locally).

So I've just merged everything into one big index.rst.
Let me know what you think.

PS still tweaking the logo...

@goodboy goodboy force-pushed the sphinxdocs branch 2 times, most recently from a8432e4 to 1b40531 Compare February 5, 2017 18:42
@goodboy
Copy link
Contributor Author

goodboy commented Feb 5, 2017

@hpk42 @RonnyPfannschmidt @nicoddemus ok so I've addressed mostly everything and have squashed the history.

Please let me know if I'm missing anything otherwise I think this is pretty much ready to land :)

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

LGTM!

@goodboy
Copy link
Contributor Author

goodboy commented Feb 5, 2017

@nicoddemus shoot just realized I rebased this onto my master which has another PRs worth of changes on it..

Fixing that now.

Tyler Goodlet added 5 commits February 5, 2017 17:33
Covers everything in the original pluggy.py module's doc string in much
more detail with links to external resources as seemed fit.

Resolves pytest-dev#14
We now have full sphinx docs as per pytest-dev#14.
pluggy.py is the entire project and it doesn't make much sense to
describe that module's contents other then saying "this is it folks".
@nicoddemus
Copy link
Member

Great, thanks! I will let @hpk42 merge this one. 😁

@goodboy
Copy link
Contributor Author

goodboy commented Feb 8, 2017

@hpk42 any news?

I was hoping to get at least this one before I start bugging you and @RonnyPfannschmidt about my other PRs ;)

@hpk42 hpk42 merged commit 21771da into pytest-dev:master Feb 8, 2017
@hpk42
Copy link
Contributor

hpk42 commented Feb 8, 2017

thanks for the continued work. fwiw was ill for 10 days, getting better now.

@goodboy
Copy link
Contributor Author

goodboy commented Feb 8, 2017

@hpk42 no worries!
I was sick all last week as well. I guess us northerners have something going around ;)

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.

3 participants