Skip to content
This repository was archived by the owner on Oct 27, 2022. It is now read-only.

Conversation

rudigiesler
Copy link

The functionality for getting a contact by a field (eg. MSISDN) is now implemented in the contacts API, we should now implement this functionality in the client.

@rudigiesler rudigiesler self-assigned this Feb 9, 2015
@rudigiesler
Copy link
Author

Tests failing because pep8 is failing on things that this PR didn't touch. Looking into it...

@rudigiesler
Copy link
Author

Seems a newer version of pep8 that checks for more things. Updated locally and it showed the same pep8 errors. pep8 errors are now fixed.

@rudigiesler
Copy link
Author

Ready for a first review.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wondering whether we shouldn't overload .get_contact() and do something like .get_contact(*args, **kw) and then:

if not kw and len(args) == 1:
    return self._contact_by_key(args[0])
elif len(kw) == 1 and not args:
    field, value = kw.items()[0]
    return self._contact_by_field(field, value)
raise ValueError(".contact() may either be called as .get_contact(contact_key) or .get_contact(field=value)")

Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

That seems good, I'll implement that.

@hodgestar
Copy link
Contributor

Left one suggestion and some minor comments. Looks good though.

@rudigiesler
Copy link
Author

@hodgestar Changes implemented. Ready for another review.

@hodgestar
Copy link
Contributor

👍

rudigiesler pushed a commit that referenced this pull request Feb 11, 2015
…tact-by-field

add getting contact by field
@rudigiesler rudigiesler merged commit 9bca090 into develop Feb 11, 2015
@rudigiesler rudigiesler deleted the feature/issue-10-add-getting-contact-by-field branch February 11, 2015 10:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants