Add missing feature switch #78
Conversation
|
@zhenlan thoughts on changing the default behavior to throw on a missing feature? I would prefer that we do it that way. It is a breaking behavioral change, but not a breaking API change. Therefore it wouldn't require a major version bump. |
|
Discussed with @zhenlan, let's go ahead and make the default behavior to not throw for a missing feature. There is a scenario for this where a developer codes ahead of someone who is setting up configuration. |
|
@jimmyca15 Thanks for the clarification, I'll update the PR. |
josephwoodward
force-pushed
the
IgnoreMissingFeaturesSwitch
branch
from
d2aca88 to
822825f
Compare
|
@jimmyca15 Updated following your comments and now ready for review. |
| /// Controls the behavior of feature evaluation when the specified feature does not exist. | ||
| /// If missing features are not ignored, an exception will be thrown when attempting to evaluate them. | ||
| /// </summary> | ||
| public bool IgnoreMissingFeatures { get; set; } |
There was a problem hiding this comment.
I think we want this defaults to true.
| services | ||
| .Configure<FeatureManagementOptions>(options => | ||
| { | ||
| options.IgnoreMissingFeatures = true; |
There was a problem hiding this comment.
I would remove this so we can test the default behavior and ensure no regressions.
| services | ||
| .Configure<FeatureManagementOptions>(options => | ||
| { | ||
| options.IgnoreMissingFeatures = true; |
| using System.Linq; | ||
| using System.Net; | ||
| using System.Net.Http; | ||
| using System.Reflection; |
There was a problem hiding this comment.
Sorry, I may miss it as it's not very easy to tell. What we are using in the reflection namespace?
| else if (!_options.IgnoreMissingFeatures) | ||
| { | ||
| throw new FeatureManagementException(FeatureManagementError.MissingFeature, $"The feature declaration for specified feature '{feature}' was not found."); | ||
| } |
There was a problem hiding this comment.
We should log a warning even if IgnoreMissingFeatures is true. Suggest change below
else
{
string errorMessage = $"The feature declaration for specified feature '{feature}' was not found.";
if (!_options.IgnoreMissingFeatures)
{
throw new FeatureManagementException(FeatureManagementError.MissingFeature, errorMessage);
}
else
{
_logger.LogWarning(errorMessage);
}
}
3 participants
No description provided.