-
Notifications
You must be signed in to change notification settings - Fork 114
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
Added support for custom feature providers. #79
Changes from all commits
c38856f
dd42402
40ec2f5
9ac9e64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,12 +7,12 @@ | |
namespace Microsoft.FeatureManagement | ||
{ | ||
/// <summary> | ||
/// The settings that define a feature filter. | ||
/// The configuration of a feature filter. | ||
/// </summary> | ||
class FeatureFilterSettings | ||
public class FeatureFilterConfiguration | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am wondering if it will be better if we call this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason I left it as Configuration rather than definition is that there is only one definition for a feature. However the same feature filter might appear in multiple features. They would just be configured differently. So to me, this was the configuration of a feature filter rather than a definition of one. |
||
{ | ||
/// <summary> | ||
/// The name of the feature filer. | ||
/// The name of the feature filter. | ||
/// </summary> | ||
public string Name { get; set; } | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
// | ||
using System.Collections.Generic; | ||
using System.Threading.Tasks; | ||
|
||
namespace Microsoft.FeatureManagement | ||
{ | ||
/// <summary> | ||
/// A provider of feature definitions. | ||
/// </summary> | ||
public interface IFeatureDefinitionProvider | ||
{ | ||
/// <summary> | ||
/// Retrieves the definition for a given feature. | ||
/// </summary> | ||
/// <param name="featureName">The name of the feature to retrieve the definition for.</param> | ||
/// <returns>The feature's definition.</returns> | ||
Task<FeatureDefinition> GetFeatureDefinitionAsync(string featureName); | ||
|
||
/// <summary> | ||
/// Retrieves definitions for all features. | ||
/// </summary> | ||
/// <returns>An enumerator which provides asynchronous iteration over feature definitions.</returns> | ||
IAsyncEnumerable<FeatureDefinition> GetAllFeatureDefinitionsAsync(); | ||
} | ||
} |
This file was deleted.
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.
Do you like the idea that we add these two APIs so we won't always add
ConfigurationFeatureDefinitionProvider
to DI and make it easier for users? Or is this too much and we can always add in the future?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.
I'd go with add in the future for that one.
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.
Especially if in the future we get a request to provide the capability to pull feature settings from an external source. Then it will point out that maybe the API that you proposed could help with the discoverability that we already offer that option.
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.
Agreed. Makes sense.