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

Provided docstrings for client.py #11

merged 18 commits into from
Nov 13, 2013

Conversation

lenards
Copy link
Contributor

@lenards lenards commented Nov 12, 2013

Relates to #5

These docstrings are provided with Sphinx-style info field list formatting. The intended target of the documentation from the docstrings is "readthedocs" and it uses Sphinx.

The voice/tone of the documentation may not be satisfactory so only this file has been done to seek feedback.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.52%) when pulling fe8841d on ajl/docstr-issue-5 into 5852368 on master.


: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

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 62cbf70 on ajl/docstr-issue-5 into 5852368 on master.

@@ -6,9 +6,23 @@


class Client:
"""Provides an interface to KISSmetrics 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.

Alright, time to get anal! :) But only because Andy asked me to.

I like to document my classes as noun phrases -- objects are the nouns of programming after all -- and save the verbs for my methods and functions. This has the added benefit of cutting down on verbosity. Thus I'd suggest we simplify this to "Interface to the KISSmetrics tracking service" or something similar.

@ewdurbin
Copy link
Contributor

i like where this is going. keep kicking ass dudez

@lenards
Copy link
Contributor Author

lenards commented Nov 13, 2013

@njm commit d2c89c5 has the changes that were suggested in comments.

Grammar is constant challenge for me (ask the co-authors on the few papers I attempted to write). Thank you for the feedback.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d2c89c5 on ajl/docstr-issue-5 into 5852368 on master.

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

@ewdurbin
Copy link
Contributor

so @lenards you can feel happy to fix our naming conventions within client.py on this PR per the following helpful ascii diagram:

  foo://username:password@example.com:8042/over/there/index.dtb?type=animal&name=narwhal#nose
  \_/   \_______________/ \_________/ \__/            \___/ \_/ \______________________/ \__/
   |           |               |       |                |    |            |                |
   |       userinfo         hostname  port              |    |          query          fragment
   |    \________________________________/\_____________|____|/ \__/        \__/
   |                    |                          |    |    |    |          |
   |                    |                          |    |    |    |          |
scheme              authority                    path   |    |    interpretable as keys
 name   \_______________________________________________|____|/       \____/     \_____/
   |                         |                          |    |          |           |
   |                 hierarchical part                  |    |    interpretable as values
   |                                                    |    |
   |            path               interpretable as filename |
   |   ___________|____________                              |
  / \ /                        \                             |
  urn:example:animal:ferret:nose               interpretable as extension

                path
         _________|________
 scheme /                  \
  name  userinfo  hostname       query
  _|__   ___|__   ____|____   _____|_____
 /    \ /      \ /         \ /           \
 mailto:username@example.com?subject=Topic

or i'll come through in one PR and get us lined up

@lenards
Copy link
Contributor Author

lenards commented Nov 13, 2013

I merged master into my PR to get the "remove QueryString class" and all the goodness from bcurtin

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d1d4909 on ajl/docstr-issue-5 into eeed6e6 on master.

@lenards
Copy link
Contributor Author

lenards commented Nov 13, 2013

I'll get this PR in-line with all the activity once the dust settles.

@lenards
Copy link
Contributor Author

lenards commented Nov 13, 2013

@ewdurbin & @njm - this is ready for review now, it is merged with master and has all feedback incorporated.

@lenards
Copy link
Contributor Author

lenards commented Nov 13, 2013

Now it's ready (spoke too soon)

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c5ab10c on ajl/docstr-issue-5 into 61ea233 on master.

@njm
Copy link
Contributor

njm commented Nov 13, 2013

This is looking really good. My final nits are to do a quick s/$/./ on client_compat.py lines 48 and 134, and on query_string.py line 19 (since those are verb phrases). Other than that this gets a 👍 from me after Travis assures me it's all okay.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 3cd41da on ajl/docstr-issue-5 into 61ea233 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling a7f7d7b on ajl/docstr-issue-5 into 61ea233 on master.

@ghost ghost assigned ewdurbin Nov 13, 2013
@ewdurbin
Copy link
Contributor

@lenards can you add an empty new line to all docstrings that need them?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c50db42 on ajl/docstr-issue-5 into 61ea233 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c50db42 on ajl/docstr-issue-5 into 61ea233 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d8e9be0 on ajl/docstr-issue-5 into 61ea233 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d8e9be0 on ajl/docstr-issue-5 into 61ea233 on master.

ewdurbin added a commit that referenced this pull request Nov 13, 2013
Provided docstrings for ``client.py``
@ewdurbin ewdurbin merged commit 0d694b6 into master Nov 13, 2013
@ewdurbin ewdurbin deleted the ajl/docstr-issue-5 branch November 13, 2013 22:25
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.

None yet

4 participants