Skip to content
This repository has been archived by the owner on Feb 5, 2018. It is now read-only.

Adding analytics API service #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adding analytics API service #16

wants to merge 1 commit into from

Conversation

jcooter
Copy link

@jcooter jcooter commented May 4, 2017

This change isn't the ideal way to solve this problem. Ideally we'd be defining the services as the foundation of the uber_analytics plugin and building the graph and frontend around those services.

This way of doing things does as little as possible to change existing code to make it easier to backport this change to older instances of uber to be able to pull historical stats.

return {
'badges_sold': c.BADGES_SOLD,
'remaining_badges': c.REMAINING_BADGES,
'badges_price': c.BADGE_PRICE,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want this to be badges_price and not badge_price?

Copy link
Author

Choose a reason for hiding this comment

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

This code snippet was taken from uber/uber/site_sections/registration.py to bring it into a proper service, I'm not sure if badges_price is actually correct - I believe @binary1230 added this code initially.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still vote for "badge_price"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we only ever used badges_price on the magstock site. if this is a copy of what's in registration.py then you can name stuff here whatever you want.

Copy link
Contributor

@RobRuana RobRuana left a comment

Choose a reason for hiding this comment

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

Two things:

  1. I agree with Eli's comment
  2. Failing pep8:
pep8 runtests: commands[0] | pep8 uber_analytics/
uber_analytics/api.py:5:1: E302 expected 2 blank lines, found 1
uber_analytics/api.py:24:33: E231 missing whitespace after ','
uber_analytics/api.py:24:46: W292 no newline at end of file

Also, this could probably use some unit tests, but since you're probably the only one using it, I'm not going to enforce that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants