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.

Sign up for GitHub

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

Jump to bottom

Feature/service collection extensions #254

Merged
merged 7 commits into from
Jan 22, 2021
Merged

Feature/service collection extensions #254

merged 7 commits into from
Jan 22, 2021

Conversation

tomasjurasek
Copy link
Contributor

@tomasjurasek tomasjurasek commented Jan 8, 2021 •
edited
Loading

Motivation

Which issue does this fix? Fixes #240

If no issue exists, what is the fix or new feature? Were there any reasons to fix/implement things that are not obvious?

Checklist

  • Code follows coding conventions held in this repo
  • Automated tests have been added
  • Tests are passing
  • Docs have been updated (if applicable)
  • Temporary settings (e.g. variables used during development and testing) have been reverted to defaults

How to test

If manual testing is required, what are the steps?

@codecov
Copy link

codecov bot commented Jan 8, 2021 •
edited
Loading

Codecov Report

Merging #254 (eaffe33) into master (38de668) will decrease coverage by 0.86%.
The diff coverage is 70.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #254      +/-   ##
==========================================
- Coverage   80.63%   79.77%   -0.87%     
==========================================
  Files         111      116       +5     
  Lines        1611     1656      +45     
  Branches      465      466       +1     
==========================================
+ Hits         1299     1321      +22     
- Misses         88      104      +16     
- Partials      224      231       +7
Impacted Files Coverage Δ
...o.Kontent.Delivery.Caching/DeliveryCacheOptions.cs 100.00% <ø> (ø)
...n/CustomServiceProviders/AutofacServiceProvider.cs 0.00% <0.00%> (ø)
...Kontent.Delivery/Helpers/DeliveryOptionsHelpers.cs 0.00% <0.00%> (ø)
...njection/Extensions/ServiceCollectionExtensions.cs 47.36% <47.36%> (ø)
...ndencyInjection/NamedDeliveryClientCacheFactory.cs 81.48% <81.48%> (ø)
...Delivery/Extensions/ServiceCollectionExtensions.cs 91.11% <87.50%> (+2.22%) ⬆️
....DependencyInjection/NamedDeliveryClientFactory.cs 89.28% <89.28%> (ø)
...very.Abstractions/Configuration/DeliveryOptions.cs 100.00% <100.00%> (ø)
...stractions/Extensions/DeliveryOptionsExtensions.cs 100.00% <100.00%> (ø)
...ching/Extensions/DeliveryCacheOptionsExtensions.cs 100.00% <100.00%> (ø)
... and 12 more

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 38de668...54379a8. Read the comment docs.

@tomasjurasek tomasjurasek force-pushed the feature/service_collection_extensions branch from e628ccb to 0c83d09 Compare January 9, 2021 10:12
@tomasjurasek tomasjurasek marked this pull request as ready for review January 9, 2021 11:55
@tomasjurasek tomasjurasek requested a review from Enngage as a code owner January 9, 2021 11:55
petrsvihlik
petrsvihlik suggested changes Jan 12, 2021
View reviewed changes
Copy link
Contributor

@petrsvihlik petrsvihlik left a comment

Choose a reason for hiding this comment

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

I'd remove the existing implementation and replace it with an Autofac-based one (as an additional package).

@tomasjurasek
Copy link
Contributor Author

I'd remove the existing implementation and replace it with an Autofac-based one (as an additional package).

The original implementation is removed now. I also removed the 'InternalsVisibleTo' parameter for the new package and create some factories for returns these internal implementations. I need to do some tests for these new use cases.

@tomasjurasek tomasjurasek force-pushed the feature/service_collection_extensions branch from 83439a3 to 4374ca3 Compare January 13, 2021 07:51
@tomasjurasek tomasjurasek force-pushed the feature/service_collection_extensions branch from 761959f to f82a31f Compare January 13, 2021 09:41
@tomasjurasek
Copy link
Contributor Author

I'd remove the existing implementation and replace it with an Autofac-based one (as an additional package).

The original implementation is removed now. I also removed the 'InternalsVisibleTo' parameter for the new package and create some factories for returns these internal implementations. I need to do some tests for these new use cases.

The new package does not depend on internal references. The infrastructure is open for new packages around DI.

@petrsvihlik petrsvihlik added this to the v15 milestone Jan 18, 2021
petrsvihlik
petrsvihlik suggested changes Jan 18, 2021
View reviewed changes
Kentico.Kontent.Delivery/DeliveryClientFactory.cs Outdated Show resolved Hide resolved
Kentico.Kontent.Delivery.Extensions.Autofac.DependencyInjection/DeliveryClientFactory.cs Outdated Show resolved Hide resolved
Kentico.Kontent.Delivery.Extensions.Autofac.DependencyInjection/DeliveryClientFactory.cs Outdated Show resolved Hide resolved
Kentico.Kontent.Delivery.Extensions.Autofac.DependencyInjection/DeliveryClientFactory.cs Outdated Show resolved Hide resolved
Kentico.Kontent.Delivery/Extensions/DeliveryOptionsExtensions.cs Show resolved Hide resolved
Kentico.Kontent.Delivery/Helpers/DeliveryOptionsHelpers.cs Show resolved Hide resolved
...nt.Delivery.Extensions.Autofac.DependencyInjection.Tests/ServiceCollectionExtensionsTests.cs Outdated Show resolved Hide resolved
...nt.Delivery.Extensions.Autofac.DependencyInjection/Extensions/ServiceCollectionExtensions.cs Outdated Show resolved Hide resolved
@tomasjurasek tomasjurasek force-pushed the feature/service_collection_extensions branch from e3597f1 to 7e95db2 Compare January 19, 2021 10:20
@tomasjurasek
extensions package for a named client
fe254c5 
extensions package for a named client

extensions package for a named client

extensions package for a named client

extensions package for a named client

extensions package for a named client

extensions package for a named client

extensions package for a named client

extensions package for a named client
@tomasjurasek tomasjurasek force-pushed the feature/service_collection_extensions branch from 949a3d4 to fe254c5 Compare January 19, 2021 20:50
petrsvihlik
petrsvihlik suggested changes Jan 22, 2021
View reviewed changes
Kentico.Kontent.Delivery.Extensions.DependencyInjection/ICustomServiceProvider.cs Outdated
/// <summary>
/// A class for providing a custom dependency.
/// </summary>
public interface ICustomServiceProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be a bit more specific and called this INamedServiceProvider

@petrsvihlik
Copy link
Contributor

It looks good to me. Besides the one comment above, I think it's ready to be released as beta and tested.

@petrsvihlik petrsvihlik merged commit 41631cb into master Jan 22, 2021
@petrsvihlik petrsvihlik deleted the feature/service_collection_extensions branch January 22, 2021 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrsvihlik petrsvihlik petrsvihlik requested changes

@Enngage Enngage Awaiting requested review from Enngage

Assignees

@tomasjurasek tomasjurasek

Labels
enhancement refactoring
Projects
None yet
Milestone
v15 - "named clients"
Development

Successfully merging this pull request may close these issues.

Spike: An alternative ServiceCollectionExtensions for named services
2 participants
@tomasjurasek @petrsvihlik