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

Add readmes to reflection #76567

Merged
merged 11 commits into from
Oct 7, 2022
Merged

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Oct 3, 2022

Added readmes; these are fairly bare-bones for now. This is part of a larger effort (see #63401) to help external contributors determine our current investment level for a library and whether it is (un)likely changes to that library will be made.

General layout is one README.md file per folder\library where each of those links back to a libraries\README.md for the description of the "Status" column.

The "status" entries are somewhat debatable for Legacy vs Inactive especially for the assemblies that are small and essentially forward to the runtime.

@steveharter steveharter added the documentation Documentation bug or enhancement, does not impact product or test code label Oct 3, 2022
@steveharter steveharter added this to the 8.0.0 milestone Oct 3, 2022
@steveharter steveharter self-assigned this Oct 3, 2022
@ghost
Copy link

ghost commented Oct 3, 2022

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

Issue Details

Added readmes; these are fairly bare-bones for now.

General layout is one "overview" doc that links all of the other readme.md files (one per folder\library) where each of those links back to the overview for the description of the "Status" column.

The "status" entries are somewhat debatable for Legacy vs Inactive especially for the assemblies that are small and essentially forward to the runtime.

Author: steveharter
Assignees: steveharter
Labels:

documentation, area-System.Reflection

Milestone: 8.0.0

Copy link
Contributor

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

The System.Reflection.Extensions, Primitives and TypeExtensions NuGet packages should not be used by anything compatible with .NET Standard 2.0 (they will blow up the number of transitive dependencies). I would not mention them at all.

The System.Reflection.Emit* packages are needed only when targeting .NET Standard 2.0 (simplifying, actually a bit more complicated), since they became inbox in .NET Standard 2.1. Referencing them will increase dependencies (although not as badly as the other three packages). I would explain that only those targeting .NET Standard 2.0 need to target them.

src/libraries/System.Reflection.Metadata/readme.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection/overview.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection/overview.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection/overview.md Outdated Show resolved Hide resolved
@AaronRobinsonMSFT
Copy link
Member

@steveharter Are these readme files about adhering to a new policy or rather an effort to improve understanding for the community? I like the idea in principle but some of the content seems like it belongs in the official doc site rather than buried in the repo.

src/libraries/System.Reflection.Context/readme.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection.DispatchProxy/readme.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Emit/readme.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Metadata/readme.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection/overview.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection/overview.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection/overview.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection/overview.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection/readme.md Outdated Show resolved Hide resolved
@jeffhandley jeffhandley requested review from ericstj and karelz October 4, 2022 04:07
@jeffhandley
Copy link
Member

@steveharter Are these readme files about adhering to a new policy or rather an effort to improve understanding for the community?

In #63401, we decided on readme docs per library to advertise the investment status for each. With these, we can also give some technical overview info targeted at contributors (as well as team members). Some of the info does overlap with the official product docs, but the audience is intended to be different.

This is our first one of them though, so we can refine here and as we progress.

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

I applied some feedback/ideas in a pull request that I sent to your fork, @steveharter. Since the ideas are opinionated, I didn't want to just push to your branch though.

steveharter#5

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

One last review from my side, @steveharter. This time I was reviewing the content more thoroughly. I suggested some more verbiage in some of the Status and Deployment sections, drawing from @teo-tsirpanis's comment.

They're just suggestions though; so I approve either way. Thanks again for getting the first round of these in and defining the structure/pattern!

src/libraries/README.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection.TypeExtensions/README.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Extensions/README.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Primitives/README.md Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Emit/README.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
- **Legacy**
- Not under development; maintained for compatibility
- Issues are likely to be closed without fixes
- PRs are unlikely to be accepted
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- PRs are unlikely to be accepted
- PRs are unlikely to be accepted

We should clarify our stance on rock-polishing PRs (e.g. PRs enabling new analyzer and style-cop warnings) going into these libraries. There is a lot of churn from these PRs today. Is this churn still ok?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. So far, it's been case-by-case. 🤔

@steveharter steveharter merged commit f9e0bdf into dotnet:main Oct 7, 2022
@steveharter steveharter deleted the ReflectionReadme branch October 7, 2022 14:53
@ghost ghost locked as resolved and limited conversation to collaborators Nov 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection documentation Documentation bug or enhancement, does not impact product or test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants