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

Change all fields named id to more specific naming #10

Merged
merged 2 commits into from
Oct 29, 2015

Conversation

dinahshi
Copy link
Contributor

As @mittonk pointed out in #8 (comment), id is a built-in function in Python so we should parse id response fields to more specific field names such as business_id.

@@ -8,7 +8,7 @@ def __init__(self, code, msg, response):
self.code = code
self.msg = msg

self.id = response['error']['id']
self.error_id = response['error']['id']
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have any objection to self.id or business.id, or dictionaries with key id.

I was just complaining about variables or plain functions named id, which shadow the built-in function.

I'm fine with leaving this line the way it is.

@dinahshi dinahshi force-pushed the dinah/rename-id-to-more-specific branch from 03232e1 to d1ab66b Compare October 29, 2015 20:15
@@ -2,3 +2,4 @@ httplib2==0.9.2
oauth2==1.9.0.post1
pre-commit==0.6.0
pytest==2.8.1
vcrpy==1.7.4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed branches a little too quickly, ignore this line! Will remove in next commit.

@dinahshi
Copy link
Contributor Author

Toned down the id changes. I recommend looking at the overall diff rather individual commits!

@mittonk
Copy link
Contributor

mittonk commented Oct 29, 2015

Much better. Here's a squirrel with a fancy hat:
:shipit:

@dinahshi dinahshi merged commit 7dd35eb into master Oct 29, 2015
@mittonk mittonk deleted the dinah/rename-id-to-more-specific branch November 11, 2015 22:36
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

3 participants