-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add extension method for easier ResourceManagementOptions
registration (Lombiq Technologies: OCORE-208)
#17095
Conversation
src/OrchardCore/OrchardCore.ResourceManagement/ServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
/// <typeparam name="T"> | ||
/// The type of the service implementing <see cref="IConfigureOptions{ResourceManagementOptions}"/> to. | ||
/// </typeparam> | ||
public static IServiceCollection AddResourceManagementOptionsConfiguration<T>(this IServiceCollection services) |
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.
public static IServiceCollection AddResourceManagementOptionsConfiguration<T>(this IServiceCollection services) | |
public static IServiceCollection ConfigureResourceManagementOptions<T>(this IServiceCollection services) |
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.
Just one suggestion for these is enough for me to get the idea ;). However, I disagree: all of these shortcuts we have are named Add*
, and for a reason: we're adding a type to the service collection, following methods like AddTransient
. The Configure*
methods deal directly with options classes, e.g.:
services.Configure<TemplateOptions>(o =>
{
o.MemberAccessStrategy.Register<DisplayMediaFieldViewModel>();
o.MemberAccessStrategy.Register<Anchor>();
o.Filters.AddFilter("img_tag", MediaFilters.ImgTag);
})
This rather adds a service that implements IConfigureOptions<T>
, only which in turn deals with options classes.
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.
Internally you configure ResourceManifest
by adding an entry on each call
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.
Yeah, but that's not what this method is doing. If we were to swap the method in this:
services.Configure<ResourceManagementOptions>(o =>
{
_manifest = new ResourceManifest();
_manifest
.DefineScript("coming-soon")
.SetUrl("~/TheComingSoonTheme/js/scripts.min.js", "TheComingSoonTheme/js/scripts.js")
.SetVersion("6.0.5");
_manifest
.DefineStyle("coming-soon")
.SetUrl("~/TheComingSoonTheme/css/styles.min.css", "TheComingSoonTheme/css/styles.css")
.SetVersion("6.0.5");
o.ResourceManifests.Add(_manifest);
})
Then we could name it ConfigureResourceManagementOptions
and thus do:
services.ConfigureResourceManagementOptions(o =>
{
...
o.ResourceManifests.Add(_manifest);
})
There the name would be appropriate. However, not for the AddTransient<IConfigureOptions<ResourceManagementOptions>, T>()
substitution I implemented.
How about calling it AddResourceConfigurations? |
Even better, renamed. |
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.
Minor suggestions
src/OrchardCore/OrchardCore.ResourceManagement/ServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Mike Alhayek <[email protected]>
Co-authored-by: Mike Alhayek <[email protected]>
@hishamco anything else you'd like to add? |
Just minor changes and ready to merge |
Co-authored-by: Hisham Bin Ateya <[email protected]>
Co-authored-by: Hisham Bin Ateya <[email protected]>
Thanks! |
Following the pattern of all the
Add*
extensions.The implementation is in
src/OrchardCore/OrchardCore.ResourceManagement/ServiceCollectionExtensions.cs
, and the release notes are the notable changes, the rest is just search and replace.