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

Directive Injector: fix last commit to master #1396

Closed
wants to merge 2 commits into from
Closed

Directive Injector: fix last commit to master #1396

wants to merge 2 commits into from

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Aug 27, 2014

  • the first commit fix the master branch,
  • the second commits implement some feedback that was discussed on the PR

@rkirov could you please review this.

vicb added 2 commits August 27, 2014 19:24
@vicb
fix(directive injector): fix a reference to an undefined variable
f058d4d
@vicb
feat(directive injector): DiCircularDependencyError -> CircularDepend…
58fb5c3 
…encyError

And make CircularDependencyError extends appDi.CircularDependencyError
so that catching appDi.CircularDependencyError catches any circular dep
excpetion
@mhevery mhevery added cla: yes and removed cla: no labels Aug 27, 2014
@rkirov
Copy link
Contributor

rkirov commented Aug 27, 2014

Thanks for catching this. Turns out 'key' is a global function and we were throwing the error with the function.

I spoke with James about this, and he objected to the qualified import, so I created #1397 that contains the variable renaming fix only.

@rkirov rkirov closed this Aug 27, 2014
@vicb
Copy link
Contributor Author

vicb commented Aug 27, 2014

@jbdeboer what's the issue with the qualified import ? (I can update so that only CDError has a prefix`)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vicb @rkirov @mhevery @jbdeboer