-
Notifications
You must be signed in to change notification settings - Fork 458
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
AspNetCore Facility bug fixes #489
Conversation
@jonorossi or @stakx I'd appreciate it if either of you could take a look and let me know what's needed to get these changes merged and into an offical release. |
@dariuslf, I don't usually work on Windsor, so I'm not in the right position to do an in-depth code review. However, I noticed that your PR does not include any tests. It might be a good idea to add a couple of (initially failing) tests that target this code & demonstrate what bugs you're fixing here. |
@stakx, thanks for the advice. Unfortunatley I cannot easily get the tests for the AspNetCore facility running and don't know where to add test cases to demonstrate my fixes. |
@dariuslf we really do need unit tests for changes here. We need to capture the defects into unit tests to ensure the problems don't come back when more changes are made, especially that this facility has no active maintainer. If you are having problems running the |
Hi @jonorossi, I've finally added some tests. |
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.
Looks great, nice work. I've just added a few minor review comments if you wouldn't mind making those changes and we'll get it merged.
Glad to hear you want to be more involved.
src/Castle.Facilities.AspNetCore.Tests/Resolvers/FrameworkDependencyResolverTestCase.cs
Outdated
Show resolved
Hide resolved
src/Castle.Facilities.AspNetCore.Tests/WindsorRegistrationOptionsControllerTestCase.cs
Outdated
Show resolved
Hide resolved
@dariuslf also your commits are against your kollmann-informatik.de email address which isn't associated with your GitHub profile. Let me know if that wasn't intended and you need help fixing it. |
796684d
to
03facda
Compare
Hope this is good to go and would like to see this in a release soon! |
Thanks @dariuslf. |
Some small fixes that have helped me use the AspNetCore facility with aspnet core 2.2.
To allow resolving multiple implementations of the same service correctly, capture the ComponentName and use as key in the register factories.
TagHelpers automatically generated for ViewComponents were not being resolved from the framework container because the namespace doesnot begin with Microsoft.
Fixed registration of TagHelpers and ViewComponents - they were mixed up.
Fixes #478