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 markdown Readme for System.Text.Json #69015

Merged
merged 11 commits into from
Jun 10, 2022

Conversation

MSDN-WhiteKnight
Copy link
Contributor

I've decided to submit a PR for one of the popular packages to see how it would go.

Related to #59630

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

ghost commented May 7, 2022

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

Issue Details

I've decided to submit a PR for one of the popular packages to see how it would go.

Related to #59630

Author: MSDN-WhiteKnight
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis
Copy link
Member

Is dotnet/runtime is the appropriate repo to maintain product documentation? We tend to use dotnet/docs and dotnet/dotnet-api-docs for that purpose. I think it might be preferable if the NuGet simply linked to the System.Text.Json documentation instead.

If we do add a README.md to the System.Text.Json, I think it might be preferable if it contained resources specific to developing/contributing to the library, e.g. STJ-specific instructions on coding conventions, testing, etc. that might somehow deviate from the repo-wide dev workflow guide.

cc @ViktorHofer @jkotas

@MSDN-WhiteKnight
Copy link
Contributor Author

I think it's useful to have embedded markdown readme, mainly to help users who come from search engines into Nuget package page understand what's this package for and how to use it. Note that there's already mini-documentation in plaintext embedded into .csproj (PackageDescription); this change enables to use Markdown so it could contain links and formatting. I can remove example if it's too hard to maintain it. I was thinking it's good to add quick usage example on package page, that helps to start using library.

If we do add a README.md to the System.Text.Json, I think it might be preferable if it contained resources specific to developing/contributing to the library

The main point of this is to display the readme on Nuget gallery page, it should be user-oriented doc. Including contributor-oriented docs would be against the intended purpose. Well, such docs could also be added into the repo, but they don't need to be embedded into package.

@eiriktsarpalis
Copy link
Member

I can remove example if it's too hard to maintain it.

If it's meant to function as a token code sample, then it probably won't require maintenance. But if it's just a token code sample, arguably a link to the up-to-date documentation is more valuable.

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Looks good in principle, although I'd just remove the Example & Commonly Used Types sections. With the library constantly evolving (e.g. inclusion of sourcegen last year) I think it will be difficult to keep up-to-date. The link to the docs should provide relevant context.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 8, 2022
@ViktorHofer
Copy link
Member

cc @danmoseley @carlossanlop regarding embedding documenting directly into our packages.

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 8, 2022
Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Thanks!

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Thank you

@eiriktsarpalis eiriktsarpalis merged commit 5efc1bb into dotnet:main Jun 10, 2022
@eiriktsarpalis
Copy link
Member

Many thanks @MSDN-WhiteKnight 👍

@MSDN-WhiteKnight MSDN-WhiteKnight deleted the patch-1 branch June 10, 2022 16:00
@ghost ghost locked as resolved and limited conversation to collaborators Jul 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json 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.

4 participants