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

Provided docstrings for client.py #11

Merged
merged 18 commits into from
Nov 13, 2013
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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
12 changes: 12 additions & 0 deletions KISSmetrics/__init__.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,22 @@
# -*- coding: utf-8 -*-

#: Current URI for all Tracking service endpoints.
Copy link
Contributor

Choose a reason for hiding this comment

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

Default host for tracking service endpoint

TRACKING_HOSTNAME = 'trk.kissmetrics.com'
#: Default protocol for inbound request to the Tracking service.
Copy link
Contributor

Choose a reason for hiding this comment

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

Default protocol for tracking service requests

Copy link
Contributor

Choose a reason for hiding this comment

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

per documentation and RFCs i just read... protocol is scheme in this case

TRACKING_PROTOCOL = 'http'

#: The URI for the endpoint to record events via the Tracking service.
Copy link
Contributor

Choose a reason for hiding this comment

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

"URI to record events via the tracking service" maybe? I generally try to omit articles (i.e. "the", "a"/"an") from my documentation since they are pretty low on information value. Same goes for the following few constants.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should call these paths if we want to be correct. we can let the docs fly through like this and I'm happy to do a pass through cleaning up our naming conventions afterwards

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I'm curious, is there any good reason to actually expose these? In other words, is there a legitimate scenario where a user would want to send an event request to somewhere other than /e? If not I suggest we eliminate the uri (path) parameters from the client methods. I'm working on some misc. refactoring and can bundle that together.

Copy link
Contributor

Choose a reason for hiding this comment

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

two fold. test coverage and the fact that we DO support multiple environments such as our staging environment and a pilot program which will offer dedicated infrastructure for very large customers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. I figured there was a use case in there somewhere, thanks.

#:
#: :see: http://support.kissmetrics.com/apis/specifications#recording-an-event
RECORD_URI = 'e'
#: The URI for the endpoint to set a property via the Tracking service.
#:
#: :see: http://support.kissmetrics.com/apis/specifications#setting-properties
SET_URI = 's'
#: The URI for the endpoint that aliases identities performing events via the
#: Tracking service.
#:
#: :see: http://support.kissmetrics.com/apis/specifications#aliasing-users
ALIAS_URI = 'a'

__author__ = 'Ernest W. Durbin III <ewdurbin@gmail.com>'
Expand Down
88 changes: 88 additions & 0 deletions KISSmetrics/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,26 @@


class Client:
"""provides operations for reporting occurrences within back-end
systems to KISSmetrics via either HTTP or HTTPS. It allows for
recording events, setting properties, and aliasing identities.

"""
def __init__(self, key, trk_host=KISSmetrics.TRACKING_HOSTNAME,
trk_proto=KISSmetrics.TRACKING_PROTOCOL):
"""Constructs instance given the API `key` for the KISSmetrics
product.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very close to how I would render it, with two small caveats. I phrase my function docstrings as imperatives (analogous to Git commits); thus "Construct..." instead of "Constructs." This again helps to keep things terse, and there's a general preference for it in the community. I'd also suggest we drop the code face from "key", and then of course correct the grammar.

Also one small nit. __init__ doesn't technically construct an object -- that's the job of __new__; __init__'s duty is to assign initial values to freshly created objects.

All this said, I'd recommend something like the following: "Initialize the client for use with the given KISSmetrics API key." Or something along those lines at least.


:param key: the API key is found on the "KISSmetrics Settings"
for a product.
:type key: str

:param trk_host: tracking URL for all requests; defaults
production tracking service.
:param trk_proto: the protocol for requests; either be HTTP or
Copy link
Contributor

Choose a reason for hiding this comment

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

accepted values are http or https

HTTPS.

"""
self.key = key
self.trk_host = trk_host
if trk_proto not in ['http', 'https']:
Expand All @@ -17,10 +34,35 @@ def __init__(self, key, trk_host=KISSmetrics.TRACKING_HOSTNAME,
self.http = PoolManager()

def url(self, query_string):
"""Handles formating the URL requests.

:param query_string: the key-value pairs to include
:type query_string: dict

:returns: a formatted, valid URL given the protocol, host &
query string
:rtype: str

"""
return '%s://%s/%s' % (self.trk_proto, self.trk_host, query_string)

def record(self, person, event, properties={}, timestamp=None,
uri=KISSmetrics.RECORD_URI):
"""Records `event` for `person` with any `properties`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Record or Records?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected: d1d4909


:param person: the individual performing the `event`
:param event: the `event` name that was performed
:param properties: any additional data to include
:type properties: dict
:param timestamp: when the `event` was performed; optional for
back-dating
:param uri: HTTP endpoint to use; defaults to
``KISSmetrics.RECORD_URI``

:returns: an HTTP response for the request
:rtype: `urllib3.response.HTTPResponse`

"""
this_request = request.record(self.key, person, event,
timestamp=timestamp,
properties=properties, uri=uri)
Expand All @@ -29,12 +71,58 @@ def record(self, person, event, properties={}, timestamp=None,

def set(self, person, properties={}, timestamp=None,
uri=KISSmetrics.SET_URI):
"""Sets a property (or properties) for a `person`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Set or Sets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected: d1d4909


:param person: the individual to associated properties with
:param properties: key-value pairs to associated with `person`
:type properties: dict
:param timestamp: when the `event` was performed; optional for
back-dating
:param uri: HTTP endpoint to use; defaults to
``KISSmetrics.SET_URI``

:returns: an HTTP response for the request
:rtype: `urllib3.response.HTTPResponse`

"""
this_request = request.set(self.key, person, timestamp=timestamp,
properties=properties, uri=uri)
url = self.url(this_request)
return self.http.request('GET', url)

def alias(self, person, identity, uri=KISSmetrics.ALIAS_URI):
"""Creates a mapping from ``person`` to ``identity`` such actions
done by either are resolved to the same person.

Note the direction of the mapping is ``person`` to ``identity``
(so ``person`` is also known as ``identity`` or ``person`` =>
``identity`` when looking at it as "source => target")

:param person: consider as same individual ``identity``; the
source of the alias operation
:type person: str or unicode
:param identity: consider as an alias of ``person``; the target
of the alias operation
:type identity: str or unicode
:param uri: HTTP endpoint to use; defaults to
``KISSmetrics.ALIAS_URI``

:returns: an HTTP response for the request
:rtype: `urllib3.response.HTTPResponse`

Aliasing is not a reversible operation. When aliasing to an
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this documentation 👍

identity, take care not to use a session identifier or any other
value that is not relatively stable (a value that will not
change per request or per session).

When consulting the Aliasing documentation, `person` corresponds
to ``query_string.PERSON_PARAM`` and `identity` corresponds to
``query_string.ALIAS_PARAM``.

For more information see the API Specifications on `Aliasing
<http://support.kissmetrics.com/apis/specifications.html#aliasing-users>`_.

"""
this_request = request.alias(self.key, person, identity, uri=uri)
url = self.url(this_request)
return self.http.request('GET', url)