Skip to content

Replace user ID usage with account ID#51

Merged
horgh merged 5 commits intomasterfrom
greg/user-id-to-account-id
Feb 13, 2018
Merged

Replace user ID usage with account ID#51
horgh merged 5 commits intomasterfrom
greg/user-id-to-account-id

Conversation

@oschwald
Copy link
Member

No description provided.

@coveralls
Copy link

coveralls commented Feb 12, 2018

Coverage Status

Coverage increased (+0.02%) to 98.94% when pulling ec20d27 on greg/user-id-to-account-id into cb96185 on master.

There is probably some code cleanup that could be done, but it probably
isn't worth doing that now. When we eventually drop 2.7 support, we can
make everything more modern.
@oschwald oschwald force-pushed the greg/user-id-to-account-id branch from 2fee8f1 to ad69357 Compare February 13, 2018 15:55
Copy link
Contributor

@horgh horgh left a comment

Choose a reason for hiding this comment

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

Long live the new Python

:param account_id: Your MaxMind account ID.
:param license_key: Your MaxMind license key.

Go to https://www.maxmind.com/en/my_license_key to see your MaxMind
Copy link
Contributor

Choose a reason for hiding this comment

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

On the next line we say "User ID". Should we update that too?


def __init__(self,
user_id,
account_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any concern that this might break backwards compatibility?

I am not very familiar with Python, but it looks like named parameters are supported:

>>> client = geoip2.webservice.Client(account_id=1, license_key='test123')
>>> client = geoip2.webservice.Client(user_id=1, license_key='test123')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__() got an unexpected keyword argument 'user_id'

Maybe it's uncommon enough that we don't need to worry though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that is a good point. Calling it like that would be pretty uncommon for parameters that are not optional, but I'll see if I can come up with a solution that works.

@oschwald oschwald force-pushed the greg/user-id-to-account-id branch from 1f96145 to ec20d27 Compare February 13, 2018 21:54
@horgh horgh merged commit 3434742 into master Feb 13, 2018
@horgh horgh deleted the greg/user-id-to-account-id branch February 13, 2018 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants