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

docs: add missing document to microsoft.extensions.options #76121

Conversation

ikesnowy
Copy link
Contributor

I couldn't find some of the apis, maybe they are no longer valid, or I missed something.

  • M:Microsoft.Extensions.Options.OptionsWrapper`1.Add(System.String,`0) (No such method in OptionsWrapper)
  • M:Microsoft.Extensions.Options.OptionsWrapper`1.Remove(System.String) (No such method in OptionsWrapper)
  • M:Microsoft.Extensions.Options.OptionsCache`1.#ctor (OptionsCache uses default constructor)
  • M:Microsoft.Extensions.Options.ValidateOptionsResult.#ctor (ValidateOptionsResult uses default constructor)
  • T:Microsoft.Extensions.Options.OptionsSnapshot (No such type)

Contributes to #43919

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 24, 2022
@ghost
Copy link

ghost commented Sep 24, 2022

Tagging subscribers to this area: @dotnet/area-extensions-options
See info in area-owners.md if you want to be subscribed.

Issue Details

I couldn't find some of the apis, maybe they are no longer valid, or I missed something.

  • M:Microsoft.Extensions.Options.OptionsWrapper`1.Add(System.String,`0) (No such method in OptionsWrapper)
  • M:Microsoft.Extensions.Options.OptionsWrapper`1.Remove(System.String) (No such method in OptionsWrapper)
  • M:Microsoft.Extensions.Options.OptionsCache`1.#ctor (OptionsCache uses default constructor)
  • M:Microsoft.Extensions.Options.ValidateOptionsResult.#ctor (ValidateOptionsResult uses default constructor)
  • T:Microsoft.Extensions.Options.OptionsSnapshot (No such type)

Contributes to #43919

Author: ikesnowy
Assignees: -
Labels:

area-Extensions-Options

Milestone: -

@ikesnowy
Copy link
Contributor Author

@carlossanlop asked about reviewer but I don't have the permission.

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Left a few suggestions for you to consider.

/// <param name="name">The name of <typeparamref name="TOptions"/> instance to create.</param>
/// <returns>The created <typeparamref name="TOptions"/> instance with thw given <paramref name="name"/>.</returns>
/// <exception cref="OptionsValidationException">One or more <see cref="IValidateOptions{TOptions}"/> return failed <see cref="ValidateOptionsResult"/> when validating the <typeparamref name="TOptions"/> instance been created.</exception>
/// <exception cref="MissingMethodException">The <typeparamref name="TOptions"/> does not have a public parameterless constructor or <typeparamref name="TOptions"/> is <see langword="abstract"/>.</exception>
Copy link
Member

@tarekgh tarekgh Sep 26, 2022

Choose a reason for hiding this comment

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

Just wondering, do we mention the exceptions in the interface docs? I think the exception is mainly thrown from the actual interface implementation. This comment applies to other places in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

That is a very good question, I don't think we should have exceptions here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I removed the exceptions from interface docs.

@tarekgh
Copy link
Member

tarekgh commented Sep 26, 2022

@ikesnowy thanks for the PR.

I added a little comment/question. Other than @carlossanlop comments, LGTM.

@tarekgh
Copy link
Member

tarekgh commented Sep 27, 2022

@carlossanlop if you are ok with the changes we can merge this one.

@tarekgh tarekgh merged commit 4aa4d28 into dotnet:main Oct 6, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Options community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants