-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add check for facet provider requirements #5073
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to merge.
@MrGizmo123 I pushed a PR slightly changing the error log and adding throwing an |
- Facet1Provider requires Facet2Provider but was added before it so far
Seems like the tests failed because the |
I added a test case that tests that if facet providers are added in an incorrect order, an exception is thrown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been over 2 months now waiting for a review and while I would've loved to get one from @keturn who originally requested this feature, I believe it's more important to finally get this improvement in. We can always still revert or modify things afterwards in case we overlooked or misunderstood something about the feature request.
Contains
Fixes #4924
Todo