Skip to content
This repository has been archived by the owner on Feb 22, 2018. It is now read-only.

feat(directive injector): DiCircularDependencyError -> CircularDependenc... #1399

Closed
wants to merge 1 commit into from

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Aug 27, 2014

...yError

And make CircularDependencyError extends di.CircularDependencyError
so that catching di.CircularDependencyError catches any circular dep
error

/cc @rkirov @jbdeboer

@vicb
Copy link
Contributor Author

vicb commented Aug 28, 2014

@rkirov I've further modified the PR (and remove the LGTM status):

  • It's a pain to have CircularDependencyError defined in two different libs, you have to disambiguate each time you use such a type. I've renamed the directive injector one to _CircularDependencyError because it is not intended to be used directly but by using the original di CircularDependencyError
  • I've updated the test to assert that the exception thrown is an instance of CircularDependencyError which is all what this PR is about
  • I have tweaked the _CircularDependencyError error message to look like the one from CircularDependencyError
  • I have made some variables private

Could you please take an other look ?

…dencyError

And make _CircularDependencyError extends CircularDependencyError
so that catching CircularDependencyError catches any circular dep
error
@vicb
Copy link
Contributor Author

vicb commented Aug 28, 2014

@rkirov the PR should be really ready now (the where closure has been updated)

@jbdeboer
Copy link
Contributor

@mhevery It looks like the labels script is buggy, as this PR has both action:review and action:merge. I've removed action:merge manually.

@vicb vicb closed this in 9f46fb9 Aug 29, 2014
vicb added a commit that referenced this pull request Aug 29, 2014
…dencyError

And make _CircularDependencyError extends CircularDependencyError
so that catching CircularDependencyError catches any circular dep
error

Closes #1399
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

4 participants