Make InsertProvider idempotent and remove reflection

[ronaldbarendse]

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open
• I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
• I have provided test coverage for my change (where applicable)

Description

As mentioned in #247 (comment), InsertProvider doesn't behave as expected and isn't idempotent: inserting a provider to a specific index only changed it to that index if it wasn't already added at a lower index:

builder.ClearProviders();
builder.AddProvider<PhysicalFileSystemProvider>();
builder.InsertProvider<MockImageProvider>(0); // Adds it before PhysicalFileSystemProvider (correct)
builder.InsertProvider<MockImageProvider>(1); // Doesn't update it to index 1 (incorrect)

builder.ClearProviders();
builder.AddProvider<PhysicalFileSystemProvider>();
builder.InsertProvider<MockImageProvider>(1); // Adds it after PhysicalFileSystemProvider (correct)
builder.InsertProvider<MockImageProvider>(0); // Inserts it before PhysicalFileSystemProvider (correct)

This PR also renames the generic type argument to TImplementation, because the some names were hard to distinguish with the interface name (e.g. TCacheHash and ICacheHash) and this aligns with the naming used on the service collection extensions.

---

I've also added a PostConfigure() extension method, so you can easily run code after the final state of the options instance is processed. This will allow users to e.g. easily ensure the directory used by the PhysicalFileSystemProvider is always created (as mentioned in #246 (comment)):

services.AddImageSharp().PostConfigure<PhysicalFileSystemProviderOptions>(options =>
{
    if (!string.IsNullOrEmpty(options.ProviderRootPath))
    {
        Directory.CreateDirectory(options.ProviderRootPath);
    }
})

[codecov]

Codecov Report

Merging #249 (ec87477) into main (72b9368) will decrease coverage by 0%.
The diff coverage is 91%.

@@         Coverage Diff         @@
##           main   #249   +/-   ##
===================================
- Coverage    85%    85%   -1%     
===================================
  Files        75     75           
  Lines      2044   2030   -14     
  Branches    297    297           
===================================
- Hits       1749   1737   -12     
+ Misses      212    210    -2     
  Partials     83     83           

Flag	Coverage Δ
unittests	85% <91%> (-1%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...DependencyInjection/ImageSharpBuilderExtensions.cs	95% <91%> (-3%)	⬇️
...ching/LruCache/ConcurrentTLruCache{TKey,TValue}.cs	46% <0%> (+2%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72b9368...ec87477. Read the comment docs.

[JimBobSquarePants]

I'd say revert everything but the actual fix. Thanks for that. 👍

You'd save yourself (and myself) a LOT of time if you just chatted first before opening a PR.

[JimBobSquarePants]

TParser is better here. Even T is better than TImplementation which is wordy and doesn't add value.

[ronaldbarendse]

I hesitated between changing it to T or TImplementation because the latter is indeed a bit wordy and doesn't add the same value as when used in the service collection extension methods (that have <TService, TImplementation>).

I've mentioned my reasoning for this in the PR description and would actually prefer to change it to T instead, if you're okay with that:

This PR also renames the generic type argument to TImplementation, because the some names were hard to distinguish with the interface name (e.g. TCacheHash and ICacheHash) and this aligns with the naming used on the service collection extensions.

[JimBobSquarePants]

The existing naming is a convention we use throughout the Six Labors libraries e.g. TPixel and works well for us. It’s far more consistent than the .NET runtime and I’d like to keep it as is.

[JimBobSquarePants]

What benefit do you see to this rather than having the explicit Configure<T> methods for each type?

PostConfigure<T> is designed to be called from within extension methods to add services to ensure that those options are not overwritten. It doesn't really have a place in the fluent builder API.

[ronaldbarendse]

I wasn't sure why Configure<T>() was added to IImageSharpBuilder at first, as it's just a proxy to IServiceCollection, but it makes it a lot easier to configure ImageSharp related options in a fluent way (although you can configure any options).

Maybe it's indeed a sensible idea to add overloads for the specific ImageSharp option types, so it easier to discover relevant options, although that would mean the Azure and S3 packages will also need to provide their own overloads for their types. You could also introduce an decorator interface and limit the generic type to e.g. where TOptions : IImageSharpOptions, but that would make discovery of the available ImageSharp options a bit harder again. What's your opinion on this?

As for PostConfigure<T>(), that basically does the same as Configure<T>(), but is invoked last, ensuring you have the final state of the options (in e.g. case multiple configure calls are done).

[ronaldbarendse]

Had a quick look and you can't create overloads for Configure<T>(this IImageSharpBuilder builder, IConfiguration config) that specify specific ImageSharp options, so the only way to limit the configure methods on IImageSharpBuilder is to create a marker interface...

I think only allowing ImageSharp related configuration on the IImageSharpBuilder is actually a better API design and you can still (post)configure the options directly on the IServiceCollection if you want...

[JimBobSquarePants]

Marker interfaces are a code smell.

Configure is fine as it is, the flexibility is a strength and it works really well when building the configuration sequentially. The examples in the Samples startup are indicative of that strength as does the config in the the unit tests.

[JimBobSquarePants]

Thanks for the fix 👍