Improve getResults docstring to follow google documentation

[lsetiawan]

Overview

This PR relates to #97. It applies google docstring standard to getResults mostly.

Also fixes a bit of getAffiliations docstring. For example, removing :obj: and changing read to ReadODM2.

Demo

[ocefpaf]

I am 👍 to remove the :obj:. Linking to the standard library there is an overkill.

PS: you can push the HTML to your forked gh-pages and add a link here like this, http://ocefpaf.github.io/ODM2PythonAPI/ so we can see the actual page instead of a screen shot.
(Also easier to do than pasting the figure 😉)

[emiliom]

Guys, thanks for all the progress you've made on the odm2api sphinx docs system in the last 24 hours!!

One comment on this PR, but it's really a more general convention comment/question: I think I'd prefer not to have text that says what the default parameter values are, and let the reader just see it from the function definition. It's more work to add text, more error prone, and adds what I think is unnecessary text clutter. @ocefpaf, what do you think?

[emiliom]

I am 👍 to remove the :obj:. Linking to the standard library there is an overkill.

👍

PS: you can push the HTML to your forked gh-pages and add a link here like this, http://ocefpaf.github.io/ODM2PythonAPI/ so we can see the actual page instead of a screen shot.
(Also easier to do than pasting the figure 😉)

Yes, please!

[lsetiawan]

PS: you can push the HTML to your forked gh-pages and add a link here like this, http://ocefpaf.github.io/ODM2PythonAPI/ so we can see the actual page instead of a screen shot.
(Also easier to do than pasting the figure 😉)

How do I do this?

[emiliom]

Is it helpful to have the module (ReadODM2) shown in the examples?? Seems like visual clutter to me.

This is a general comment, not specific to getAffiliations

[ocefpaf]

Is it helpful to have the module (ReadODM2) shown in the examples?? Seems like visual clutter to me.

ReadODM2 is the parent class of this method, right? I guessed that read was an instance of that class. If getAffiliations is a static method it should be called with ReadODM2().getAffiliations, no? If not I guess that the confusing part is naming the instance the same name as the parent class, but I am OK with the examples.

[lsetiawan]

@emiliom, @ocefpaf I am following what pycharm is telling me. I get unresolved reference warning.. I can change to whatever.

If getAffiliations is a static method it should be called with ReadODM2().getAffiliations, no?

getAffiliations is not a static method.

Here's an example of the usage:

• https://github.com/ODM2/ODM2PythonAPI/blob/master/Examples/Sample.py#L38. ReadODM2 get instantiated and then the functions are used.

[emiliom]

Ah, I'd forgotten that getAffiliations was a ReadODM2 class method. This makes sense now. I was assuming (w/o checking, sorry ...) that ReadODM2 was simply a module reference.

My bad.

[ocefpaf]

I am following what pycharm is telling me

That is b/c pycharm is probably trying to lint that fragment of code. The original example probably omitted the instantiation of the class.

read = ReadODM2()

[emiliom]

So keep ReadODM2?

Yes.

[ocefpaf]

So keep ReadODM2?

If that is a class method, yes. I need to check the code but it does not look like it.
See https://stackoverflow.com/questions/17134653/difference-between-class-and-instance-methods

If that is an instance method than you need to add more to the example, like:

read = ReadODM2()
read.<method>

[emiliom]

My executive decision: keep it for now, so we can move forward with the PR. We can discuss again later, elsewhere, to lay out general conventions.

[ocefpaf]

Like I suspected it is not a class method. Without an instance this cannot be called:

Wrong:

from odm2api.ODM2.services import ReadODM2
ReadODM2.getAffiliations(ids=[39,40])
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-82d16725b458> in <module>()
----> 1 ReadODM2.getAffiliations(ids=[39,40])

TypeError: unbound method getAffiliations() must be called with ReadODM2 instance as first argument (got nothing instead)

Mocked "correct" example by instantiating an instance:

r = ReadODM2(session)
r.getAffiliations(ids=[39,40])
<returns ids from session>

[emiliom]

Thanks for the follow-up, @ocefpaf. @lsetiawan, let's start compiling elsewhere a list of conventions we'll follow.

[ocefpaf]

How do I do this?

You have to create a gh-pages branch on your fork with the HTML code (and nothing more), and push it.

[emiliom]

One comment on this PR, but it's really a more general convention comment/question: I think I'd prefer not to have text that says what the default parameter values are, and let the reader just see it from the function definition. It's more work to add text, more error prone, and adds what -- I think -- is unnecessary text clutter. @ocefpaf, what do you think?

I suspect this comment of mine may have gotten lost in the barrage of other comments; pasting it just in case.

[ocefpaf]

I suspect this comment of mine may have gotten lost in the barrage of other comments; pasting it just in case.

Nope. I it just took some reading of the docstring to understand what you meant.

One comment on this PR, but it's really a more general convention comment/question: I think I'd prefer not to have text that says what the default parameter values are, and let the reader just see it from the function definition.

That is a matter of preference but I usually like to specify the default in the docstrings to explain them.

It's more work to add text, more error prone, and adds what I think is unnecessary text clutter. @ocefpaf, what do you think?

In this particular case they are all None and there is no exaplanation. Also, the eventual explanation would be redundant with the" optional" text already there, so I agree they are not needed.

[lsetiawan]

That is a matter of preference but I usually like to specify the default in the docstrings to explain them.

If function has a default value other than None then I will specify them. Otherwise I won't.

In this particular case they are all None and there is no exaplanation. Also, the eventual explanation would be redundant with the" optional" text already there, so I agree they are not needed.

This sounds good. Thanks for input! 😄

[emiliom]

That is a matter of preference but I usually like to specify the default in the docstrings to explain them.

If function has a default value other than None then I will specify them. Otherwise I won't.

In this particular case they are all None and there is no exaplanation. Also, the eventual explanation would be redundant with the" optional" text already there, so I agree they are not needed.

Executive decision, for now (again, to move forward with the PR): remove from the text from the docstring in this case. But more generally, let's also not include that text when there is no explanation in the docstring. Of course, we can revisit that "decision" later!!

[emiliom]

Travis-CI is passing. Looks like it's ready to merge, and we have no outstanding comments?

If so, @ocefpaf please merge. I have a meeting in 2 minutes.

[emiliom]

Let me know when these changes (plus, I assume, more modules being listed) are available at http://odm2.github.io/ODM2PythonAPI/
I'm really psyched to see them in action, and to know that we have the workflow in place to update the docs!

[ocefpaf]

Let me know when these changes (plus, I assume, more modules being listed) are available at http://odm2.github.io/ODM2PythonAPI/

They are up as soon as Travis-CI finished the "docs" task, see https://travis-ci.org/ODM2/ODM2PythonAPI

So it should be 2-3 minutes more 😄

[emiliom]

So it should be 2-3 minutes more

I don't see it updated at http://odm2.github.io/ODM2PythonAPI/ or http://odm2.github.io/ODM2PythonAPI/py-modindex.html

[ocefpaf]

I don't think @lsetiawan changed the docs, only the docstrings.

@lsetiawan do you want to wait until you write everything to add it to the modules.rst?
If so I guess we should also remove my bare bones example there. (I added it just for testing.)

[emiliom]

@lsetiawan do you want to wait until you write everything to add it to the modules.rst?
If so I guess we should also remove my bare bones example there. (I added it just for testing.)

I'd rather do a partial test first, with a partial listing in modules.rst, to ensure the workflow is in place and Don and I know what to do. Then next week we can update odm2api docstrings, and the overall set of sphinx documents, at our own pace.

[emiliom]

with a partial listing in modules.rst

.... including the two methods whose docstrings @lsetiawan has fleshed out

[lsetiawan]

You can see here: https://lsetiawan.github.io/ODM2PythonAPI/modules.html#odm2api.ODM2.services.readService.ReadODM2.getResults

[lsetiawan]

@ocefpaf can we only expose certain functions not whole module?

[ocefpaf]

@ocefpaf can we only expose certain functions not whole module?

You probably can by overriding the auto-discover functionality of sphinx.
Or using a few tricks like :undoc-members:, see http://www.sphinx-doc.org/en/stable/ext/autodoc.html

[emiliom]

You can see here: https://lsetiawan.github.io/ODM2PythonAPI/modules.html#odm2api.ODM2.services.readService.ReadODM2.getResults

Woo-hoo!!! Awesome! 🕺

can we only expose certain functions not whole module?

@lsetiawan, is there a reason why you'd like to "hide" some functions? I think we should be exposing everything right now, the whole module. If your concern is the lack of a docstring (or a docstring not yet properly formatted), that's not a reason to hide it.

[lsetiawan]

If your concern is the lack of a docstring (or a docstring not yet properly formatted), that's not a reason to hide it.

If that's fine then we can expose this whole module.

[emiliom]

If your concern is the lack of a docstring (or a docstring not yet properly formatted), that's not a reason to hide it.

If that's fine then we can expose this whole module.

Yup, let's do that! This will be a work in progress, but already a vast improvement in odm2api documentation!!

[emiliom]

You can see here: https://lsetiawan.github.io/ODM2PythonAPI/modules.html#odm2api.ODM2.services.readService.ReadODM2.getResults

Woo-hoo!!! Awesome!

I missed the fact that was on your fork ... Still, great to see that we're almost there, but I'll hold off on the wild ecstatic dancing until it's on http://odm2.github.io/ODM2PythonAPI/index.html