Remove no-op ImageSharpConfiguration and setup action#245
Conversation
Codecov Report
@@ Coverage Diff @@
## main #245 +/- ##
===================================
Coverage 85% 85%
===================================
Files 75 74 -1
Lines 2030 2028 -2
Branches 297 297
===================================
Hits 1734 1734
+ Misses 212 210 -2
Partials 84 84
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| return builder; | ||
| } | ||
|
|
||
| private static IImageSharpBuilder AddDefaultServices(this IImageSharpBuilder builder) |
There was a problem hiding this comment.
Although default implementations are also set for IRequestParser, IImageCache, ICacheKey and ICacheHash, those are actually required for ImageSharpMiddleware to activate/resolve from the service container and can easily be replaced with the respective set-methods.
Maybe this extension method should be made public and the AddImageSharp() shouldn't call it by default, so you can opt-in to adding these default providers/processors? And following this pattern, maybe even split it into AddDefaultProviders() and AddDefaultProcessors()?
There was a problem hiding this comment.
This would also solve the issue I described in PR #185: the order of registering IImageProviders is important, but can't be (easily) changed. Having a separate method to add the default provider after your own will remove the need to remove and re-add them...
There was a problem hiding this comment.
I don’t want to change registration methods at this time.
There was a problem hiding this comment.
The whole point of the default implementation is that out of the box it works.
There was a problem hiding this comment.
I wasn't sure about this either and agree that it's better to have everything working OOTB 😄 I see you've also made inserting providers easier in #247, so this is now less of an issue...
JimBobSquarePants
left a comment
JimBobSquarePants
left a comment
There was a problem hiding this comment.
Please revert all other changes and remove the no-op ImageSharpConfiguration class only.
The samples serve two purposes and should remain untouched.
- Education
- API sense checking.
| /// </summary> | ||
| /// <param name="services">The collection of service desscriptors.</param> | ||
| public void ConfigureServices(IServiceCollection services) | ||
| { |
There was a problem hiding this comment.
You still need to revert all the changes bar removing ImageSharpConfiguration
There was a problem hiding this comment.
The default configuration is already documented in the ConfigureCustomServicesAndCustomOptions() method and having the sample site use the minimal, default AddImageSharp() will ensure it's actually running with the default configuration that's added by the ImageSharpBuilder 👍🏻
There was a problem hiding this comment.
It makes it much more difficult for me to play around with though. We have unit tests for DI.
| Action<ImageSharpMiddlewareOptions> setupAction) | ||
| private static IImageSharpBuilder AddDefaultServices(this IImageSharpBuilder builder) | ||
| { | ||
| builder.Services.Configure(setupAction); |
There was a problem hiding this comment.
One thing this call did was internally call AddOptions(), so the generic options interfaces are registered and can be resolved from the service container.
Looking at other extension methods in ASP.NET Core, they do explicitly call AddOptions() as well, so let's make sure ImageSharp.Web does the same (even if it's probably already added anyway, we can't be sure though).
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm sorry but I simply do not follow. Why are you changing any of this? We're only supposed to be removing the no-op ImageSharpConfiguration
2 participants
Prerequisites
Description
I noticed a no-op
ImageSharpConfigurationinstance and an empty lambda for the setup action was registered: both don't add anything besides additional logic that's run when getting anIOptions<ImageSharpMiddlewareOptions>.