Alex/check post by alexcouper · Pull Request #11 · apibyexample/abe-python · GitHub
Skip to content

Alex/check post - #11

Merged
txels merged 2 commits into
apibyexample:masterfrom
alexcouper:alex/check-post
Nov 17, 2015
Merged

Alex/check post#11
txels merged 2 commits into
apibyexample:masterfrom
alexcouper:alex/check-post

Conversation

@alexcouper

Copy link
Copy Markdown
Collaborator

This solves #10

However, it also does one other things that maybe the abe-python project doesn't want:

  • A backwards incompatible change to the function definition of assert_matches_sample to remove data that is derivable (ie ''url").

@txels

txels commented Nov 9, 2015

Copy link
Copy Markdown
Member

Comment thread README.md Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To be honest, these two are inherited from the top level endpoint definition and don't need to be duplicated here, since the URL is already a valid one and not a pattern (e.g. '/accounts/{someone}/').

@alexcouper

Copy link
Copy Markdown
Collaborator Author

Having method and even URL (when it's not parameterised and thus doesn't vary across examples (1)) from the endpoint definition in every sample request is already implemented in ABE-python (https://github.com/apibyexample/abe-python/blob/master/abe/mocks.py#L28) and arguably not needed for what you are after.

@txels Are you saying that the ABE format already supports overriding of both METHOD and URL within each request example? I didn't see that in the spec readme. Is there a more full definition?

If overriding these properties is already part of the spec then I'm not actually proposing any changes here.

@txels

txels commented Nov 9, 2015

Copy link
Copy Markdown
Member

No. I meant to say that ABE-python already prefills method in your sample from the one in the main description. I probably misunderstood that you were after having the request section in the example fully populated before comparing to actual request.

@txels

txels commented Nov 9, 2015

Copy link
Copy Markdown
Member

To be fair to you, the spec doesn't mandate either, as it's not that tightly defined. But I would argue against using a single file for multiple methods for the reasons detailed above.

@txels

txels commented Nov 9, 2015

Copy link
Copy Markdown
Member

:) maybe calling it a spec at this point is a bit pretentious - we should definitely be more formal about it cc/ @nwhite89 .

@nwhite89

nwhite89 commented Nov 9, 2015

Copy link
Copy Markdown
Member

I would like this to be kept as un-bias to any type of API, I suggest moving this discussion out as you said @txels into perhaps an issue within abe-spec as this is an feels like an overall implementation idea of the entirety of ABE rather than specific to abs-python.

@alexcouper

Copy link
Copy Markdown
Collaborator Author

I have amended the commit to work with the current format.

txels added a commit that referenced this pull request Nov 17, 2015
@txels
txels merged commit 4f3b980 into apibyexample:master Nov 17, 2015
Comment thread abe/unittest.py

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm I realise now that the signature of this method has changed in a backwards-incompatible way, thus breaking all my ABE-python tests in another project...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that was one of the controversial things about this PR that was up for discussion (see the PR description).

Should be relatively painless to fix - and I think you'll agree (assuming your other project is a django project) is less error prone. Before this change you could have posted anywhere you wanted, and just passed in a url that worked to the assertion.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TBF I missed that bit during review. I think this works better. We'll add some release notes to the next release when this comes out.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

0.3.0 release I presume?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will be released together with #14

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sounds good

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