Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat(MockHttpExpectation): matchData supports function as data #2981

Closed
wants to merge 2 commits into from
Closed

feat(MockHttpExpectation): matchData supports function as data #2981

wants to merge 2 commits into from

Conversation

kenspirit
Copy link
Contributor

Add support for passing function as validating data:

  • To avoid hacking test method of RegExp
  • Optionally overwrite toString method of fn to show validation tips

Add support for passing function as validating data:
 - To avoid hacking test method of RegExp
 - Optionally overwrite toString method of fn to show validation tips
@pkozlowski-opensource
Copy link
Member

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
  • Feature improves existing core functionality
  • API is compatible with existing Angular apis and relevant standards (if applicable)
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • PR contains e2e tests (if suitable)
  • PR contains documentation update
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required [commit message

@pkozlowski-opensource
Copy link
Member

@kenspirit Thnx for this PR. Could you go over the attached checklist? Main points are the CLA and documentation update.

@kenspirit
Copy link
Contributor Author

Thx for the update. I have signed the CLA previously before making this PR.
I have used name "Ken Chen" with email "chengusky@gmail.com" and signed electrically (online). But I couldn't find any document to reference back.

@petebacondarwin
Copy link
Member

Since we are duck-typing regexp then you can get the required functionality already with:

var exp = new MockHttpExpectation('POST', '/url', { test: dataValidator });

and you could add a toString method if you wanted...

var exp = new MockHttpExpectation('POST', '/url', { test: dataValidator, toString: function() { return 'Some Tip'; } });

Is there any real benefit in adding this or just update the documentation? [Devil's Advocate here]

@kenspirit
Copy link
Contributor Author

Hi @petebacondarwin ,

True that duck-typing works and that is also what I love about JS, the flexibility.
However, it just seems the duck-typing and manually enclosing a function into another object just for that looks not beautiful here.

It looks like the API is not complete to support function type and we need to "hack" it somehow.
I know this is a tiny change and it doesn't seem to have some real benefit.
I just saw supporting this would be more direct and natural.

@kenspirit
Copy link
Contributor Author

Hi @petebacondarwin ,

Almost forgot. There is one real benefit.
Providing a toString() method in the duck-typing object doesn't work.
Because AngularJs will treat the object as a Object instead of Function which doesn't use the toString() to print in console.

    function prettyPrint(data) {
      return (angular.isString(data) || angular.isFunction(data) || data instanceof RegExp)
          ? data
          : angular.toJson(data);
    }

@petebacondarwin
Copy link
Member

:-) Good stuff

On 17 July 2013 21:50, Ken Chen notifications@github.com wrote:

Hi @petebacondarwin https://github.com/petebacondarwin ,

Almost forgot. There is one real benefit.
Providing a toString() method in the duck-typing object doesn't work.
Because AngularJs will treat the object as a Object instead of Function
which doesn't use the toString() to print in console.

function prettyPrint(data) {
  return (angular.isString(data) || angular.isFunction(data) || data instanceof RegExp)
      ? data
      : angular.toJson(data);
}


Reply to this email directly or view it on GitHubhttps://github.com//pull/2981#issuecomment-21143810
.

@kenspirit
Copy link
Contributor Author

Hi @pkozlowski-opensource ,

Is that I should also update the documentation so that this PR can be accepted?
Just want to see what further action I need to take?

 - change param desc for when, whenPost, whenPut,
   expect, expectPost, expectPut, expectPATCH
petebacondarwin pushed a commit to petebacondarwin/angular.js that referenced this pull request Aug 6, 2013
Add support for passing function as validating data:
 - To avoid hacking test method of RegExp
 - Optionally overwrite `toString` method of fn to show validation tips
 - change docs: param description for `when`, `whenPost`, `whenPut`,
   `expect`, `expectPost`, `expectPut`, `expectPATCH`

Closes: angular#2981
@petebacondarwin
Copy link
Member

@kenspirit - Thanks for this PR. Landed on master as 08daa77

@kenspirit
Copy link
Contributor Author

Thanks. @petebacondarwin
It's my very first PR being accepted and it's for AngularJS. Cannot tell how happy I am.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants