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

Improve getResults docstring to follow google documentation #105

Merged
merged 2 commits into from
Sep 29, 2017
Merged

Improve getResults docstring to follow google documentation #105

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b329231
Apply google standard
lsetiawan Sep 29, 2017
629a352
Addresses repetition for default values
lsetiawan Sep 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 37 additions & 19 deletions odm2api/ODM2/services/readService.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,19 +549,19 @@ def getAffiliations(self, ids=None, personfirst=None, personlast=None, orgcode=N
all Affiliation objects in the database will be returned.

Args:
ids (:obj:`list`, optional): List of AffiliationIDs. Defaults to None.
personfirst (:obj:`str`, optional): Person First Name. Defaults to None.
personlast (:obj:`str`, optional): Person Last Name. Defaults to None.
orgcode (:obj:`str`, optional): Organization Code. Defaults to None.
ids (list, optional): List of AffiliationIDs.
personfirst (str, optional): Person First Name.
personlast (str, optional): Person Last Name.
orgcode (str, optional): Organization Code.

Returns:
list: List of Affiliation objects

Examples:
>>> read.getAffiliations(ids=[39,40])
>>> read.getAffiliations(personfirst='John',
>>> ReadODM2.getAffiliations(ids=[39,40])
>>> ReadODM2.getAffiliations(personfirst='John',
... personlast='Smith')
>>> read.getAffiliations(orgcode='Acme')
>>> ReadODM2.getAffiliations(orgcode='Acme')
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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:

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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()

Copy link
Member

Choose a reason for hiding this comment

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

So keep ReadODM2?

Yes.

Copy link
Member

Choose a reason for hiding this comment

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

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>

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ocefpaf ocefpaf Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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>

Copy link
Member

Choose a reason for hiding this comment

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

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


"""
q = self._session.query(Affiliations)
Expand All @@ -588,18 +588,36 @@ def getResults(self, ids=None, type=None, uuids=None, actionid=None, simulationi
variableid=None, siteid=None):

# TODO what if user sends in both type and actionid vs just actionid
"""
getResults(self, ids=None, type=None, uuids=None, actionid=None, simulationid=None, sfid=None,
variableid=None, siteid=None)
* Pass nothing - returns a list of all Results objects
* Pass a list of ResultID - returns a single Results object
* Pass a ResultType - returns a list of Result objects of that type. must be from ResultTypeCV
* Pass a list of UUIDs -
* Pass an ActionID - returns a single Results object
* Pass a Sampling Feature ID- returns a list of objects with that Sampling Feature ID
* Pass a Variable ID - returns a list of results with that Variable ID
* Pass a Simulation ID - return a list of results that were generated by that simulation
* Pass a Site ID - return a list of results from that site location, through the related features table.
"""Retrieve a list of Result objects.

If no arguments are passed to the function, or their values are None,
all Result objects in the database will be returned.

Args:
ids (list, optional): List of ResultIDs.
type (str, optional): Type of Result from
`controlled vocabulary name <http://vocabulary.odm2.org/resulttype/>`_.
uuids (list, optional): List of UUIDs string.
actionid (int, optional): ActionID.
simulationid (int, optional): SimulationID.
sfid (int, optional): SamplingFeatureID.
variableid (int, optional): VariableID.
siteid (int, optional): SiteID.

Returns:
list: List of Result objects

Examples:
>>> ReadODM2.getResults(ids=[39,40])
>>> ReadODM2.getResults(type='Time series coverage')
>>> ReadODM2.getResults(sfid=65)
>>> ReadODM2.getResults(uuids=['a6f114f1-5416-4606-ae10-23be32dbc202',
... '5396fdf3-ceb3-46b6-aaf9-454a37278bb4'])
>>> ReadODM2.getResults(simulationid=50)
>>> ReadODM2.getResults(siteid=6)
>>> ReadODM2.getResults(variableid=7)
>>> ReadODM2.getResults(actionid=20)

"""

query = self._session.query(Results)
Expand Down