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

feat($injector): report circularity in circular dependency error message #7500

Conversation

teropa
Copy link
Contributor

@teropa teropa commented May 17, 2014

Request Type: feature

How to reproduce:

Component(s): di

Impact: medium

Complexity: small

This issue is related to:

Detailed Description:

Change the error message for a circular dependency to display the full
circle back to the first service being instantiated, so that the problem
is obvious:

a <- b <- a

The previous message stopped one dependency short of the full
circle:

b <- a

Other Comments:

Changes the content of the cdep error message, which may be considered
a breaking change.

Change the error message for a circular dependency to display the full
circle back to the first service being instantiated, so that the problem
is obvious. The previous message stopped one dependency short of the full
circle.

Changes the content of the cdep error message, which may be considered
a breaking change.
@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#7500)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@teropa
Copy link
Contributor Author

teropa commented May 17, 2014

When I see a circular dependency error, the problem usually isn't obvious to me at first glance. This change would make it obvious.

@mgol
Copy link
Member

mgol commented May 18, 2014

+1000 for that!

@benjamingr
Copy link
Contributor

+1 for this, would help.

@teropa teropa added cla: yes and removed cla: no labels May 19, 2014
@caitp caitp added this to the 1.3.0-beta.10 milestone May 21, 2014
@caitp
Copy link
Contributor

caitp commented May 21, 2014

@IgorMinar this is pretty tiny, just a change to how the error is reported, it looks okay to me.

I think you could even mark this as a fix. Anyways, since it is marked as a feature, I'd like to get someone else to look at it as well, but it LGTM

@btford
Copy link
Contributor

btford commented Jun 3, 2014

I think it's reasonable. +1 for this being a "fix."

@btford btford self-assigned this Jun 3, 2014
rodyhaddad pushed a commit that referenced this pull request Jun 13, 2014
Change the error message for a circular dependency to display the full
circle back to the first service being instantiated, so that the problem
is obvious. The previous message stopped one dependency short of the full
circle.

Changes the content of the cdep error message, which may be considered
a breaking change.

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

Successfully merging this pull request may close these issues.

7 participants