Skip to content

Conversation

alexcouper
Copy link
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
Copy link
Member

txels commented Nov 9, 2015

Thanks for this.

The extension of sample matching to not only response but also request is a welcome one, that was missing from this implementation.

Re: the reinterpretation of the ABE format, I'd say:

  • Changes to the ABE spec should be proposed and discussed as issues in https://github.com/apibyexample/abe-spec and not in this repo, since they impact other tools and not just abe-python, and would require reviewing by other people not following ABE-python (e.g. @nwhite89)
  • The suggested change, if I understand correctly, is one where the subject of an ABE file is a "resource" rather than an endpoint. I would argue that change implies more than just being able to override the method in examples:
    • It would also imply that the documentation in the main section becomes that of the resource, the description of the endpoint (e.g. GET) would need now a place to live (and there may be a variety of samples for every enpoint) etc..
    • It would bias ABE towards a ReST architectural style, which may not be everyone's use case
  • 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.

Grouping together endpoints that "belong together" for whatever reason (e.g. because they represent the same resource [collection] in ReST) is something we certainly didn't cover when defining the spec. There is a case for defining a mechanism for that (for example if you want to write a tool to generate documentation out of ABE files), although the way I always thought of it was not within an ABE file, but as additional information either in extra files, naming conventions on your files, or a directory structure. We have no defined proposal for this.

(1) There is a difference between url at the top level (where it typically more formally defines a pattern) and in each sample, where if it exists (it's not required) it has to be a concrete valid URL to the service you are ABE'ing.

To be fair to you, there is some looseness in the definition of the ABE format that could do with some tightening (cc/ @nwhite89 in case we want to take some action there).

README.md Outdated
Copy link
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
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
Copy link
Member

txels commented Nov 9, 2015

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
Copy link
Member

txels commented Nov 9, 2015

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
Copy link
Member

txels commented Nov 9, 2015

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

@nwhite89
Copy link
Member

nwhite89 commented Nov 9, 2015

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
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
Copy link
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
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
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
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
Member

Choose a reason for hiding this comment

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

Yes.

Copy link
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
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