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

Attempt to add options for TryAdd and Replace #22

Merged
merged 8 commits into from
Apr 7, 2017

Conversation

adamhathcock
Copy link
Contributor

I'm not sure I like how I did this but it seems to be correct.

The replace implementation removes all instances of the ServiceType. The Replace extension Microsoft uses only removes the first instance.

@adamhathcock
Copy link
Contributor Author

Also need to replace by implementation type.

This is all because there is no equivalency checking on the ServiceCollection and no equality on ServiceDescriptor

@khellang
Copy link
Owner

khellang commented Apr 6, 2017

What would you use ReplaceImplementationType for? There's no guarantee there will be an implementation type for some descriptors.

@khellang
Copy link
Owner

khellang commented Apr 6, 2017

I'm not a fan of the API letting you call both UseTryAdd and ReplaceServiceTypes/ReplaceImplementationTypes because they're mutually exclusive.

If we try to boil it down, it comes down to three different collision behaviors:

  1. Skip - Only add the service if there's no existing service registered, basically TryAdd.
  2. Append - Adds the service anyway, will probably only be resolve through IEnumerable.
  3. Replace - Replace the existing service registrations.

Could we, instead of having three different methods, have one; UsingCollisionBehavior that takes an enum with one of these values? I'm not sure "collision behavior" is a good name, though.

When it comes to the actual replacement, I'm not sure we want to support "implementation type" because of the reason mentioned above.

@adamhathcock
Copy link
Contributor Author

I had been thinking of an enum really but wanted to prove out things working and show you as nothing seemed good.

ImplementationType is important to me because my use case (MediatR) registers via the MediatR interfaces with implementation types. I need to replace some to not be Transient but to be Singletons. I realize ImplementationType could be null but it shouldn't be a big deal. It just wouldn't replace it.

@adamhathcock
Copy link
Contributor Author

I have two enums now. One for the overall registration behavior, the second is a flags for options for just the replacement behavior. I realize the replacement behavior is tricky but I need the option.

@khellang
Copy link
Owner

khellang commented Apr 7, 2017

There's one issue I still feel we need to iron out; the current API let's you pass a replacement strategy, even though it makes no sense for 2/3 options. I propose we do something like the following:

public abstract class RegistrationStrategy
{
    public static readonly RegistrationStrategy Skip = new SkipStrategy();

    public static readonly RegistrationStrategy Append = new AppendStrategy();

    public static RegistrationStrategy Replace(ReplacementBehavior behavior) => new ReplaceStrategy(behavior);

    public abstract void Apply(IServiceCollection services, ServiceDescriptor descriptor);

    private sealed class SkipStrategy : RegistrationStrategy
    {
        public override void Apply(IServiceCollection services, ServiceDescriptor descriptor) => services.TryAdd(descriptor);
    }

    private sealed class AppendStrategy : RegistrationStrategy
    {
        public override void Apply(IServiceCollection services, ServiceDescriptor descriptor) => services.Add(descriptor);
    }

    private class ReplaceStrategy : RegistrationStrategy
    {
        public ReplaceStrategy(ReplacementBehavior behavior)
        {
            Behavior = behavior;
        }

        private ReplacementBehavior Behavior { get; }

        public override void Apply(IServiceCollection services, ServiceDescriptor descriptor)
        {
            var behavior = Behavior;

            if (behavior == ReplacementBehavior.Default)
            {
                behavior = ReplacementBehavior.ServiceType;
            }

            for (var i = services.Count - 1; i >= 0; i--)
            {
                if ((behavior.HasFlag(ReplacementBehavior.ServiceType) && services[i].ServiceType == descriptor.ServiceType)
                    || (behavior.HasFlag(ReplacementBehavior.ImplementationType) && services[i].ImplementationType == descriptor.ImplementationType))
                {
                    services.RemoveAt(i);
                }
            }

            services.Add(descriptor);
        }
    }
}

[Flags]
public enum ReplacementBehavior
{
    Default = 0,
    ServiceType = 1,
    ImplementationType = 2,
    All = ServiceType | ImplementationType
}

It'll be like an enum, but on steroids 😉 It'll also let people implement their own and gives nice IntelliSense:

image

@adamhathcock
Copy link
Contributor Author

Added your code. It's much prettier!

behavior = ReplacementBehavior.ServiceType;
}

for (var i = services.Count - 1; i >= 0; i--)
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reason behind removing from back to front?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I can modify the collection in a loop. It shrinks as it moves along.

If I use foreach, I'd get a collection modified exception. If I looped forward then a removal would cause the index to be off by one.


for (var i = services.Count - 1; i >= 0; i--)
{
if ((behavior.HasFlag(ReplacementBehavior.ServiceType) && services[i].ServiceType == descriptor.ServiceType)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure it matters, but HasFlag is painfully slow, so I usually try to stay away from it on principle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change it I guess. I like it for clarity because I always screw up bitwise stuff I feel.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, screw it. Let's leave it 😄

@khellang khellang merged commit 451c124 into khellang:master Apr 7, 2017
@khellang
Copy link
Owner

khellang commented Apr 7, 2017

@adamhathcock Thanks for an awesome PR, once again! ❤️

I'll push out a version ASAP 👍

@adamhathcock
Copy link
Contributor Author

Thanks! I use this library all the time.

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.

2 participants