fix for issue #38 #39

Merged
merged 5 commits into from Mar 12, 2014

Projects

None yet

3 participants

@talam
Contributor
talam commented Mar 3, 2014

I think this is what you were looking for. An error message now gets logged to terminal.

@dirn
Member
dirn commented Mar 3, 2014

I think it would be better to put the logging closer to the code that interacts with the API (i.e., APIWrapper), that way we don't need to repeat it everywhere we call one of these methods. There is also some benefit to performing this logging action in one place should different parts of the application use different logging settings.

@dirn
Member
dirn commented Mar 3, 2014

Also don't forget to add yourself to AUTHORS.rst.

@jonafato
Member
jonafato commented Mar 3, 2014

It seems to me like the logic represented here would handle @dirn's comments and more if moved into APIWrapper._get. Specifically, if we replace line 36 with the log-and-return-empty-list logic, we'll be covered on both the current and any future request types. This also ensures that one request's failure doesn't mask another's success.

@dirn
Member
dirn commented Mar 5, 2014

There seems to be a lot going on here now. I think this pull request should focus on just #38, logging the error and returning an empty list.

Also don't forget to add yourself to AUTHORS.rst.

@dirn dirn closed this Mar 5, 2014
@dirn dirn reopened this Mar 5, 2014
@talam talam removed all the extra stuff not related to issue #38. fixed up the lo…
…g-and-return-empty-logic on line 36 per jonafato and dirns suggestions
19d3695
@talam
Contributor
talam commented Mar 5, 2014

Yea, sorry about that. I was trying to commit back to my branch for my own reference and didn't realize it also was getting pulled into this request. Updated my commit and removed all the extra stuff and now it should return an empty list.

@dirn
Member
dirn commented Mar 5, 2014

Almost there. meetup_api_logger is undefined.

@talam
Contributor
talam commented Mar 6, 2014

Lets try this one last time. Changed the incorrect variable reference to _logger now.

@dirn dirn merged commit 35d569a into NYCPython:master Mar 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment