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

Added new API source using the giantbomb.com API #3

Closed
wants to merge 3 commits into from

Conversation

@zerok
Copy link
Contributor

zerok commented Mar 2, 2013

This one uses the Giantbomb.com API to fetch platform release dates and original prices and puts them into perspective with Federal Reserve CPI data (to calculate USD inflation), matplotlib (to generate to generate an output graph) and tablib (to generate a CSV file).

@econchick
Copy link
Owner

econchick commented Mar 3, 2013

I love this idea. So. Great.

import requests
import logging
import tablib
import urllib2

This comment has been minimized.

@econchick

econchick Mar 3, 2013 Owner

why are you electing to use urllib2 for one object (line 62) while you could use requests?

This comment has been minimized.

@zerok

zerok Mar 3, 2013 Author Contributor

Requests supports gzipped content by default with makes chunked downloading a bit of a mess. But an updated version will now use requests for consistency sake :-)

if current_year is not None and current_year not in self.year_cpi:
self.year_cpi[current_year] = sum(year_cpi) / len(year_cpi)

def get_adapted_price(self, price, year, current_year=None):

This comment has been minimized.

@econchick

econchick Mar 3, 2013 Owner

simple docstring?

This comment has been minimized.

@zerok

zerok Mar 3, 2013 Author Contributor

For inline-comments I prefer actual comments and reserve docstrings for API documentation. Or what do you mean? :-)

This comment has been minimized.

@econchick

econchick Mar 3, 2013 Owner

well you use docstrings in the previous two functions - why not this one?

Just an FYI - this the 2nd of the 5 tutorials, so erring on the side over documentation, hand holding, and no "magic" would be better.

This comment has been minimized.

@zerok

zerok Mar 3, 2013 Author Contributor

Ah, that's what you meant ^_^ The diff shown in the e-mail kind of suggested you were talking about the load_from_file method and not the get_adapted_price method :-)

Will correct that and look through the code again for any "magic" :-)

This comment has been minimized.

@econchick

econchick Mar 3, 2013 Owner

You're awesome :)

On Sun, Mar 3, 2013 at 7:51 AM, Horst Gutmann notifications@github.comwrote:

In apis/lib/full_source/platform_pricing.py:

  •        # The moment we reach a new year, we have to reset the CPI data
    
  •        # and calculate the average CPI of the current_year.
    
  •        if current_year != year:
    
  •            if current_year is not None:
    
  •                self.year_cpi[current_year] = sum(year_cpi) / len(year_cpi)
    
  •            year_cpi = []
    
  •            current_year = year
    
  •        year_cpi.append(cpi)
    
  •    # We have to do the calculation once again for the last year in the
    
  •    # dataset.
    
  •    if current_year is not None and current_year not in self.year_cpi:
    
  •        self.year_cpi[current_year] = sum(year_cpi) / len(year_cpi)
    
  • def get_adapted_price(self, price, year, current_year=None):

Ah, that's what you meant ^_^ The diff shown in the e-mail kind of
suggested you were talking about the load_from_file method and not the
get_adapted_price method :-)

Will correct that and look through the code again for any "magic" :-)


Reply to this email directly or view it on GitHubhttps://github.com//pull/3/files#r3217657
.

@zerok
Copy link
Contributor Author

zerok commented Mar 3, 2013

Now with 50% more hand-holding :-)

@econchick
Copy link
Owner

econchick commented Mar 3, 2013

These comments are great!!

On Sun, Mar 3, 2013 at 8:21 AM, Horst Gutmann notifications@github.comwrote:

Now with 50% more hand-holding :-)


Reply to this email directly or view it on GitHubhttps://github.com//pull/3#issuecomment-14349296
.

@econchick
Copy link
Owner

econchick commented Mar 5, 2013

Okay - for your review, let me know your thoughts on how the tutorial reads: https://gist.github.com/econchick/29a6a892a54c743f3268

:)

@econchick
Copy link
Owner

econchick commented Mar 9, 2013

Manually merged! Still working on editing/fine-tuning the actual tutorial part. :)

@econchick econchick closed this Mar 9, 2013
econchick pushed a commit that referenced this pull request Mar 19, 2013
Selecting a random quote more elegantly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.