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.

Sign up for GitHub

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

Jump to bottom

Fix docs datastore examples #2039

Merged
merged 1 commit into from
Aug 17, 2016
Merged

Fix docs datastore examples #2039

merged 1 commit into from
Aug 17, 2016

Conversation

daspecster
Copy link
Contributor

Fixes #2037.

This is a partial fix for the datastore docstring issue.

The original issue was actually, in part, due to the examples being in the datastore.Connection class. Sphinx put them all on one page for some reason.

@daspecster daspecster added the docs label Aug 1, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 1, 2016
dhermes
dhermes reviewed Aug 1, 2016
View reviewed changes
scripts/generate_json_docs.py

if element.docstring:
if not isinstance(element, pdoc.Class) and element.cls:
cls = element.cls.cls
clas = element.cls.cls

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Aug 1, 2016

The original issue was actually, in part, due to the examples being in the datastore.Connection class. Sphinx put them all on one page for some reason.

We have until now curated all the docs/*.rst files to make the documentation usable, so just machine generating from every module with no regard for the original hand curated docs is a bad idea.

@daspecster
Copy link
Contributor Author

I agree with you there.
TBH, after going down that road a few times I wasn't able to find a good way to get all the information from the RST. AutoDoc is pretty extensive and from what I can tell it would be a pretty big undertaking to recreate that. If there is a reasonable way to do it that I missed, I'm all for it.

That's probably a bigger topic than this PR though.

@dhermes
Copy link
Contributor

dhermes commented Aug 1, 2016

Slowly bringing the two approaches in sync would've been the right move. There is a way (one that is employed by oauth2client) to just have the files themselves be the source of truth.

@daspecster
Copy link
Contributor Author

daspecster commented Aug 1, 2016 •
edited
Loading

I'm not really following I guess.

sphinx-apidoc --separate --force -o docs/source oauth2client

Creates RST files with the autodoc directives in them. I guess you could grab the module references from that? But that's not a huge win in this case I don't think? I could be wrong or missing something though.

@dhermes
Copy link
Contributor

dhermes commented Aug 1, 2016

The point is that in that regime we don't care what is in the RST files. Using sphinx-apidoc makes the Python modules the single source of truth.

@daspecster
Copy link
Contributor Author

daspecster commented Aug 1, 2016 •
edited by dhermes
Loading

Basically instead of get_public_modules?

@dhermes
Copy link
Contributor

dhermes commented Aug 1, 2016

I'm not referring to your handrolled solution, so get_public_modules is kind of a side-concern. I'm just saying if we went that route then it would easier map onto the concept that you've come up with (which I think was to map onto whatever has been done in gcloud-common)

@daspecster
Copy link
Contributor Author

The gcloud-common site structure is different from our sphinx structure. I asked for multiple class support but didn't gain any traction. That would help match things up to what we have in Sphinx.

In anycase, I moved the example code to client.py since that's in the top level nav for the site docs. We should probably think of a strategy for handling this stuff though.

@dhermes
Copy link
Contributor

dhermes commented Aug 2, 2016

That sounds fine, we should have an issue for this discussion. Your docstring changes seem fine, how should I vet the docgen stuff in scripts/generate_json_docs.py?

@daspecster daspecster mentioned this pull request Aug 2, 2016
Closed
@daspecster
Copy link
Contributor Author

Sure, although generate_json_docs.py is not great code and should be refactored and cleaned up.

I made #2043 to talk about what I think you were saying before.

@dhermes
Copy link
Contributor

dhermes commented Aug 2, 2016

Thanks

@daspecster
Copy link
Contributor Author

@dhermes, I didn't delete the docs from connection.py. I thought it would be good to leave it for now. Otherwise I think I got the other issues you mentioned.

@daspecster daspecster force-pushed the fix-docs-datastore-examples branch 2 times, most recently from 3dc290f to 75baf31 Compare August 17, 2016 17:02
dhermes
dhermes reviewed Aug 17, 2016
View reviewed changes
scripts/generate_json_docs.py

# Hack for old-style classes
if str(cls)[0] != '<':
cls = '<class \'' + str(cls) + '\'>'
if str(klass)[0] != '<':

This comment was marked as spam.

Sign in to view

dhermes
dhermes reviewed Aug 17, 2016
View reviewed changes
scripts/generate_json_docs.py
@@ -27,6 +28,9 @@
from verify_included_modules import get_public_modules


docstring_test_parser = doctest.DocTestParser()

This comment was marked as spam.

Sign in to view

@tseaver
Copy link
Contributor

tseaver commented Aug 17, 2016

LGTM FWIW

dhermes
dhermes reviewed Aug 17, 2016
View reviewed changes
scripts/generate_json_docs.py
example_str += '%s' % (example.source,)
example_str += '%s' % (example.want,)

return example_str.replace('<', '&lt;').replace('>', '&gt;')

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@daspecster
Move examples to query docstring.
ac356df 
Pull examples with doctest and exit less early for Method parsing.

Move example docstring from connection.py->client.py

Change clas to klass.
@daspecster daspecster mentioned this pull request Aug 17, 2016
Closed
@dhermes
Copy link
Contributor

dhermes commented Aug 17, 2016

LGTM. @daspecster on future PRs can you hold off on squashing until the PR is ready to merge? It makes it a lot harder to verify which fixes have been made during code review.

@daspecster
Copy link
Contributor Author

@dhermes ah ok good point! Thanks!

@daspecster daspecster merged commit 536377a into googleapis:master Aug 17, 2016
@daspecster
Copy link
Contributor Author

@tseaver I missed your use the six wrappers comment until after merge. But I changed that line to cgi.escape() anyway. Were you saying there is a six wrapper for cgi.escape? I didn't see anything like that in six.

@tseaver
Copy link
Contributor

tseaver commented Aug 17, 2016

@daspecster Nope, I was mistaken: six does not have the wrapper.

@dhermes
Copy link
Contributor

dhermes commented Aug 17, 2016

Yup. six does have urllib though (if we did go the urlencode route)

@dhermes dhermes mentioned this pull request Sep 19, 2016
Merged
@daspecster daspecster deleted the fix-docs-datastore-examples branch January 24, 2017 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@tseaver tseaver

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Python documentation missing examples, etc
4 participants
@daspecster @dhermes @tseaver @googlebot