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

Initial pact-python implementation #1

Merged
merged 2 commits into from
Apr 7, 2017
Merged

Conversation

matthewbalvanz-wf
Copy link
Contributor

  • Supports creating consumer driven contracts using an external Pact mock service
  • Includes the EachLike, SomethingLike, and Term matchers
  • Is setup to package itself as pact-python

This is mostly a direct copy of the project we created internally at Workiva. Because of that its larger than I prefer and we can break things up if that is desired.

There are a couple of things worth discussion:

  • The project is currently setup to enforce the code linting standards for the team that created it, which is 100% test coverage, linting of code to the PEP-8 and docstrings to the PEP-257 standard. I'm open to discussion here, but we've found it leads us to write consistently organized and reasonably well documented code.
  • I added my name and email address as author in the setup.py defining the project. While I did write the majority of the code, it feels like it might be appropriate to have something like Pact Foundation and an email of some sort, but I'm unsure what.

- Supports creating consumer driven contracts using an external Pact mock service
- Includes the EachLike, SomethingLike, and Term matchers
- Is setup to package itself as `pact-python`
@mefellows
Copy link
Member

To my untrained eye, this looks to be a pretty good start. Even if not perfect, I'd be inclined to pull this straight in to master as it gives us something to work from.

@jslvtr - what do you think?

pact/matchers.py Outdated
return term
elif isinstance(term, (six.string_types, int, float)):
return term
if isinstance(term, dict):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be elif?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. This one made it through all our internal reviews. With the return statements its functionally the same, but for consistency I think its best to change it to elif.

pact/pact.py Outdated
return self

def will_respond_with(
self, status, headers=None, body=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this newline here part of the style guide? Seems that full-length 106 would be shorter than e.g. 81.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My hunch is that during development I had more arguments before I fully understood the mock service. I'll shorten this up to one line.

@jslvtr
Copy link
Contributor

jslvtr commented Apr 1, 2017

Looks very good! I've put a couple of single comments in there just to satisfy my curiosity; other than that looks like the best start. 🚢

- Correct pact.matchers.from_term to only have one logic structure
- Fix Pact.will_respond_with’s signature to a single line
@matthewbalvanz-wf matthewbalvanz-wf merged commit 8f074a0 into master Apr 7, 2017
@matthewbalvanz-wf matthewbalvanz-wf deleted the initial-framework branch April 7, 2017 14:55
matthewbalvanz-wf pushed a commit that referenced this pull request Nov 20, 2017
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.

3 participants