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

Add package readme for Microsoft.Extensions.Configuration.Binder #77649

Merged
merged 4 commits into from
Oct 30, 2022
Merged

Add package readme for Microsoft.Extensions.Configuration.Binder #77649

merged 4 commits into from
Oct 30, 2022

Conversation

MSDN-WhiteKnight
Copy link
Contributor

Issue: #59630

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

ghost commented Oct 30, 2022

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

Issue Details

Issue: #59630

Author: MSDN-WhiteKnight
Assignees: -
Labels:

area-Extensions-Configuration, community-contribution
Milestone: -

@MSDN-WhiteKnight MSDN-WhiteKnight mentioned this pull request Oct 30, 2022
Closed
7 tasks
@build-analysis build-analysis bot mentioned this pull request Oct 30, 2022
Closed
@tarekgh
Copy link
Member

tarekgh commented Oct 30, 2022 •
edited
Loading

@MSDN-WhiteKnight thanks for submitting that. Could you please follow the same format for the readmes. Here is exampel https://github.com/dotnet/runtime/tree/main/src/libraries/Microsoft.Extensions.Caching.Abstractions#readme. Also move the readme to the root folder of the library instead of src folder.

#77413

@tarekgh tarekgh added this to the 8.0.0 milestone Oct 30, 2022
tarekgh
tarekgh reviewed Oct 30, 2022
View reviewed changes
src/libraries/Microsoft.Extensions.Configuration.Binder/src/README.md Outdated
The APIs and functionality are mature, but do get extended occasionally.

## Deployment
[Microsoft.Extensions.Configuration.Binder](https://www.nuget.org/packages/Microsoft.Extensions.Configuration.Binder/) is not included in the shared framework. The package is deployed as out-of-band (OOB) and needs to be installed into projects directly.
Copy link
Member

@tarekgh tarekgh Oct 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

is not included in the shared framework

Could you please change this to be is included in the ASP.NET core shared framework.

We'll need to fix this in the rest of readmes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume you meant "is included..."? It's actually there looking at shipped binaries. But it is not included in regular Netcoreapp shared framework. So shouldn't it be something like this:

Microsoft.Extensions.Configuration.Binder is included in ASP.NET Core shared framework, but not in regular .NET shared framework. The package is deployed as out-of-band (OOB) and needs to be installed into projects directly when not using ASP.NET Core.

Copy link
Member

@tarekgh tarekgh Oct 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Right. I have suggested some tweak in my comment #77649 (review). Let me double check if this library deployment in aspnet core. Thanks for the catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, didn't noticed the latest review suggestion when commenting.

Copy link
Member

Choose a reason for hiding this comment

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

I confirmed this library is part of ASP.NET Core Runtime

@tarekgh
Copy link
Member

tarekgh commented Oct 30, 2022

Could you please move the readme to the root of the library folder?

@tarekgh
Copy link
Member

tarekgh commented Oct 30, 2022

I left one last comment. LGTM otherwise.

@tarekgh
Copy link
Member

tarekgh commented Oct 30, 2022

Thanks @MSDN-WhiteKnight for your contribution. Are you interested in providing more readme files for Microsoft.Extensions libraries? it is easy to check which library is missing the readme.

@tarekgh
Copy link
Member

tarekgh commented Oct 30, 2022

I'll wait CI to finish then will merge this one.

@MSDN-WhiteKnight
Copy link
Contributor Author

Are you interested in providing more readme files for Microsoft.Extensions libraries? it is easy to check which library is missing the readme.

Yes, i'll pick Microsoft.Extensions.Configuration.* packages without readme then. I've also asked a question about the intended direction with readmes there: #77413 (comment) (not sure if commenting under merged PR is good idea, but i don't see the issue that discusses this).

@build-analysis build-analysis bot mentioned this pull request Oct 30, 2022
Closed
@tarekgh tarekgh merged commit 5c61ad4 into dotnet:main Oct 30, 2022
@MSDN-WhiteKnight MSDN-WhiteKnight deleted the binder-readme branch October 31, 2022 03:15
@ghost ghost locked as resolved and limited conversation to collaborators Nov 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tarekgh tarekgh tarekgh approved these changes

Assignees
No one assigned
Labels
area-Extensions-Configuration community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@MSDN-WhiteKnight @tarekgh