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

Create Parsons Mailchimp connector #239

Merged
merged 29 commits into from
May 14, 2020
Merged

Create Parsons Mailchimp connector #239

merged 29 commits into from
May 14, 2020

Conversation

SorenSpicknall
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@jburchard jburchard left a comment

Choose a reason for hiding this comment

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

Initial first pass. This is great. Only some minor changes needed, largely around comments for the documentation.

Also, please add:

  • mailchimp.rst file
  • mailchimp to the index.rst file. See other classes for examples.
  • mailchimp.py import top level __init__.py

parsons/mailchimp/mailchimp.py Outdated Show resolved Hide resolved
parsons/mailchimp/mailchimp.py Show resolved Hide resolved
parsons/mailchimp/mailchimp.py Show resolved Hide resolved
parsons/mailchimp/mailchimp.py Show resolved Hide resolved
parsons/mailchimp/mailchimp.py Show resolved Hide resolved
parsons/mailchimp/mailchimp.py Outdated Show resolved Hide resolved
parsons/mailchimp/mailchimp.py Outdated Show resolved Hide resolved
parsons/mailchimp/mailchimp.py Outdated Show resolved Hide resolved
parsons/mailchimp/mailchimp.py Show resolved Hide resolved
@SorenSpicknall
Copy link
Collaborator Author

I added RST documentation in 4b1a3e0, and resolved other specific feedback in additional commits. Could you expand on what you mean by this, though, @jburchard?

mailchimp.py import top level init.py

@jburchard
Copy link
Collaborator

jburchard commented May 9, 2020

Sure.

Just import the class into this file:

Copy link
Contributor

@eliotst eliotst left a comment

Choose a reason for hiding this comment

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

I just had the one comment. As per our conversation, it'd also be good to get some unit tests in before merging. This is great work! Thanks!

parsons/mailchimp/mailchimp.py Outdated Show resolved Hide resolved
@SorenSpicknall
Copy link
Collaborator Author

I added some testing with basic assertions, which we can expand upon down the road, and took care of the remaining feedback items. Ready for re-review.

@eliotst eliotst merged commit 189fd50 into master May 14, 2020
@eliotst eliotst deleted the mailchimp_connector branch May 14, 2020 13:14
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