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

Improve summaries for methods in AzureAppConfigurationOptions #467

Merged
merged 5 commits into from
Oct 5, 2023

Conversation

amerjusupovic
Copy link
Member

@amerjusupovic amerjusupovic commented Sep 22, 2023

Add mention of default query in the summary for AzureAppConfigurationOptions. Fix tags in summary for AzureAppConfigurationOptions.UseFeatureFlags.

@amerjusupovic amerjusupovic marked this pull request as draft September 22, 2023 22:13
@@ -118,6 +118,7 @@ internal IEnumerable<IKeyValueAdapter> Adapters
/// <summary>
/// Specify what key-values to include in the configuration provider.
/// <see cref="Select"/> can be called multiple times to include multiple sets of key-values.
/// If neither <see cref="Select"/> nor <see cref="SelectSnapshot"/> is ever called, all key-values with null label are included in the configuration provider.
Copy link
Member

Choose a reason for hiding this comment

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

The intent seems good, but it's a bit odd that it's on the Select method. I guess I would expect it on AppConfigurationOptions summary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea I agree honestly, I wasn't sure if adding it to the options summary seemed random/too much information there. Probably a bit better than this

@amerjusupovic amerjusupovic marked this pull request as ready for review September 29, 2023 18:13
Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>
@amerjusupovic amerjusupovic merged commit 34ba149 into preview Oct 5, 2023
2 checks passed
@amerjusupovic amerjusupovic deleted the ajusupovic/clarify-select-usage branch October 5, 2023 20:50
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.

If feature flags are the only filters then all keys are requested
2 participants