Skip to content
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

Fixed inspections and quick fixes registration in Castle Windsor #3845

Merged
merged 1 commit into from Mar 17, 2018

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Mar 17, 2018

Apparently some PR broke the IoC registration for inspections and quick fixes. Only ParseTreeInspections were registered correctly. Other inspections and all quick fixes were not registered at all or registered having service type of Object. This PR's purpose is to restore the functionality.

@rkapka
Copy link
Contributor Author

rkapka commented Mar 17, 2018

I'm not familiar with CW, so this fix might not be the best approach. I don't know why Base and Self services were not working.

@rkapka rkapka requested a review from MDoerner March 17, 2018 21:39
@codecov
Copy link

codecov bot commented Mar 17, 2018

Codecov Report

Merging #3845 into next will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             next   #3845      +/-   ##
=========================================
+ Coverage   55.99%     56%   +0.01%     
=========================================
  Files         827     827              
  Lines       28613   28611       -2     
=========================================
+ Hits        16020   16021       +1     
+ Misses      12593   12590       -3
Impacted Files Coverage Δ
Rubberduck.Main/Root/RubberduckIoCInstaller.cs 98.15% <100%> (-0.01%) ⬇️
...bberduck.SettingsProvider/XmlPersistanceService.cs 3.45% <0%> (+3.45%) ⬆️
...crete/ImplicitDefaultMemberAssignmentInspection.cs 5.56% <0%> (+5.56%) ⬆️

@retailcoder retailcoder merged commit 7b74b4b into rubberduck-vba:next Mar 17, 2018
Copy link
Contributor

@MDoerner MDoerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is basically right, but the Base() must be replaced with an explicit selection in all cases.

The basic problem here is that removing the BasedOn robs CW of the information what the base should be. So, it defaults to object. To use the services Base() and FromInterface() it is necessary to select via BasedOn<>.

Maybe we should switch back to that and use the If or Unless to add the experimental feature condition. Unfortunately, I do not really know whether these work as expected as they are missing from the CW documentation.

@@ -244,8 +244,7 @@ private void RegisterQuickFixes(IWindsorContainer container, Assembly[] assembli
container.Register(Classes.FromAssembly(assembly)
.IncludeNonPublicTypes()
.Where(type => type.IsBasedOn(typeof(IQuickFix)) && type.NotDisabledExperimental(_initialSettings))
.WithService.Base()
.WithService.Self()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Self() should work and is important at the places where specific quickfixes get injected.

@@ -269,7 +268,6 @@ private void RegisterParseTreeInspections(IWindsorContainer container, Assembly[
container.Register(Classes.FromAssembly(assembly)
.IncludeNonPublicTypes()
.Where(type => type.IsBasedOn(typeof(IParseTreeInspection)) && type.NotDisabledExperimental(_initialSettings))
.WithService.Base()
.WithService.Select(new[] { typeof(IInspection) })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has to be extended by typeof(IParseTreeInspection) to account for the removed Base().

@MDoerner
Copy link
Contributor

I just looked further in the CW documentation. There was no link in the part about registration by convention but in the part about registering component-by-component.

Conditional component registration

After reading this, I think we should go back to BasedOn combined with If or Unless to deal with the experimental types.

@rkapka rkapka deleted the rkapka-ioc branch April 6, 2018 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants