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

add subscriber details #18

Closed
wants to merge 2 commits into from
Closed

add subscriber details #18

wants to merge 2 commits into from

Conversation

tomzaragoza
Copy link

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling afe5c15 on tomzaragoza:createsend-add-subscribers-details into a0fe851 on campaignmonitor:master.

@tobio
Copy link
Contributor

tobio commented Nov 13, 2013

Hey @tomzaragoza,

The wrapper already provides a method to get a subscribers details. I understand that this method is operating on the instance fields which makes sense, but rather than create a new method we'd prefer updating the existing method to use the instance fields if the list_id and email_address arguments aren't supplied.

Something like

  def get(self, list_id=None, email_address=None):
    """Gets a subscriber by list ID and email address."""
    params = { "email": email_address or self.email_address }
    response = self._get("/subscribers/%s.json" % list_id or self.list_id, params=params)
    return json_to_py(response)

@tomzaragoza
Copy link
Author

Oh, didn't see that! Shall I update the pull request with your preferred way of updating the method?

@tobio
Copy link
Contributor

tobio commented Nov 13, 2013

All good. Just updating this pull request is fine, any additional commits to your createsend-add-subscribers-details branch should be automatically reflected here.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 20b7848 on tomzaragoza:createsend-add-subscribers-details into a0fe851 on campaignmonitor:master.

@jdennes
Copy link
Contributor

jdennes commented Nov 14, 2013

Could we get an additional test here please?

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.

4 participants