Basic pack functionality for Embedded Readmes by advay26-zz · Pull Request #3575 · NuGet/NuGet.Client

advay26-zz · 2020-08-12T19:23:59Z

Bug

Fix

Details: This implements basic embedded Readme functionality through nuget pack, dotnet pack and msbuild /t:pack, and does the Client-side validation for the packed Readme files.

Testing/Validation

Tests Added: Yes

…spec file

…ctory

…speed with nuget pack

…estMetadata.cs Co-authored-by: donnie-msft <[email protected]>

…ns optional

zkat

I love all the tests, and everything looks good so far! I spotted something that brought up a question for me but the rest looks good. Great work!

…rches exclusively?), added some tests to catch it out

…orms

loic-sharma · 2020-08-15T23:44:43Z

        bool Serviceable { get; }
        string Copyright { get; }
        string Icon { get; }
+        string Readme { get; }


A concern I have is that one could easily mistake Readme as the readme content instead of a path - especially for undocumented members. What do you think of renaming properties to ReadmePath and methods to GetReadmePath()?

Would this change apply to the actual nuspec element name <readme> as well, making it something like <readmePath>, or would it only apply to the names of properties and methods in the code?

Up to you. Personally I'd be fine with renaming just the properties and methods in code so that we can keep the nuspec element <readme> consistent with the <license type="file"> element.

I agree on not changing the nuspec element name. However, I'm personally unsure about how we should weigh the concerns about the Readme property name not being clear on readme contents vs path, against possible concerns about consistency between:

the nuspec <readme> element and other nuspec elements (we seem to agree that we shouldn't change this)

the nuspec <readme> element and the corresponding Readme properties in the code

Would ReadmePath properties in the code cause confusion because of the fact that the corresponding nuspec element is named <readme> and not <readmePath>? I don't necessarily have an answer to this, and I'm happy to leave this conversation open for others to weigh in too.

nkolev92

Quickly skimming through it this looks great.

You are missing dotnet pack tests though. https://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Core.FuncTests/Dotnet.Integration.Test/PackCommandTests.cs

https://github.com/NuGet/NuGet.Client/blob/dev/docs/feature-guide.md#pack-considerations

advay26-zz · 2020-08-19T23:45:01Z

You are missing dotnet pack tests though.

@nkolev92 Done! I've added new tests for dotnet pack now. Thanks for pointing it out.

* Made basic changes to classes to allow for a Readme element in the nuspec file * More changes for readme functionality * Adding the readme property if 'readme.md' is present in the base directory * Added support for dotnet pack for .csproj files * Added validation for Readmes when packing, brought dotnet pack up to speed with nuget pack * Changed variable names, changed error message for empty Readme * Removed commented code * Added a summary for the CheckForReadme method * Added support for dotnet pack for .csproj files * Added support for dotnet pack for .csproj files * Added validation for Readmes when packing, brought dotnet pack up to speed with nuget pack * Changed variable names, changed error message for empty Readme * Updated PublicAPI.Unshipped.txt files * Changed some spacing * Wrote tests for packing embedded readmes * Removed automatic packing of readmes from base directory * Used platform independent directory separating chars for testing * Update src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/ManifestMetadata.cs Co-authored-by: donnie-msft <[email protected]> * Updated PublicAPI files after file structure was modified * Reverted to 'Readme == string.Empty' so that the Readme element remains optional * Removed 1 MB file size check on pack * Updated Public API files * Updated Public API files again, builds fine locally * Fixed a bug with dir. separating chars (which seemed to affect /* searches exclusively?), added some tests to catch it out * Changed var name in one of the tests * Changed tests that were constructed incorrectly for non-Windows platforms * Added dotnet pack tests Co-authored-by: donnie-msft <[email protected]>

* Basic pack functionality for Embedded Readmes (#3575) * Made basic changes to classes to allow for a Readme element in the nuspec file * More changes for readme functionality * Adding the readme property if 'readme.md' is present in the base directory * Added support for dotnet pack for .csproj files * Added validation for Readmes when packing, brought dotnet pack up to speed with nuget pack * Changed variable names, changed error message for empty Readme * Removed commented code * Added a summary for the CheckForReadme method * Added support for dotnet pack for .csproj files * Added support for dotnet pack for .csproj files * Added validation for Readmes when packing, brought dotnet pack up to speed with nuget pack * Changed variable names, changed error message for empty Readme * Updated PublicAPI.Unshipped.txt files * Changed some spacing * Wrote tests for packing embedded readmes * Removed automatic packing of readmes from base directory * Used platform independent directory separating chars for testing * Update src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/ManifestMetadata.cs Co-authored-by: donnie-msft <[email protected]> * Updated PublicAPI files after file structure was modified * Reverted to 'Readme == string.Empty' so that the Readme element remains optional * Removed 1 MB file size check on pack * Updated Public API files * Updated Public API files again, builds fine locally * Fixed a bug with dir. separating chars (which seemed to affect /* searches exclusively?), added some tests to catch it out * Changed var name in one of the tests * Changed tests that were constructed incorrectly for non-Windows platforms * Added dotnet pack tests Co-authored-by: donnie-msft <[email protected]> * PM UI surfaces a 'View README' link to the Embedded Readme file (#3583) * Made initial changes for PM UI work for Readmes * Added ReadmeUrl element to all the relevant places, still using ProjectUrl to see an an actual link for now * URL now shows when user hovers over the link's text * Removed the placeholder URL value that used the ProjectUrl Co-authored-by: Advay Tandon <[email protected]> Co-authored-by: donnie-msft <[email protected]>

advay26 and others added 23 commits August 12, 2020 14:43

Made basic changes to classes to allow for a Readme element in the nu…

6f34978

…spec file

More changes for readme functionality

850eb0e

Adding the readme property if 'readme.md' is present in the base dire…

d153c30

…ctory

Added support for dotnet pack for .csproj files

0fd31c5

Added validation for Readmes when packing, brought dotnet pack up to …

17df178

…speed with nuget pack

Changed variable names, changed error message for empty Readme

77d2e17

Removed commented code

75f78ff

Added a summary for the CheckForReadme method

36c8327

Added support for dotnet pack for .csproj files

f9e99d0

Added support for dotnet pack for .csproj files

aa9999a

Added validation for Readmes when packing, brought dotnet pack up to …

d3e6b32

…speed with nuget pack

Changed variable names, changed error message for empty Readme

1fa0e3e

Updated PublicAPI.Unshipped.txt files

b11261c

Changed some spacing

cc870c6

Wrote tests for packing embedded readmes

c5f4016

Removed automatic packing of readmes from base directory

3aab0f1

Used platform independent directory separating chars for testing

48f1352

Update src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/Manif…

e65bf2f

…estMetadata.cs Co-authored-by: donnie-msft <[email protected]>

Updated PublicAPI files after file structure was modified

12e418f

Reverted to 'Readme == string.Empty' so that the Readme element remai…

ac846c4

…ns optional

Removed 1 MB file size check on pack

8be7319

Updated Public API files

321e0a5

Updated Public API files again, builds fine locally

11dee8f

zkat reviewed Aug 14, 2020

View reviewed changes

Comment thread src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PackageBuilder.cs

advay26 added 3 commits August 14, 2020 15:23

Fixed a bug with dir. separating chars (which seemed to affect /* sea…

0ee2569

…rches exclusively?), added some tests to catch it out

Changed var name in one of the tests

83a9af5

Changed tests that were constructed incorrectly for non-Windows platf…

db4e1e9

…orms

advay26-zz marked this pull request as ready for review August 14, 2020 21:45

advay26-zz requested review from aortiz-msft and cristinamanum as code owners August 14, 2020 21:45

advay26-zz requested review from dominoFire, donnie-msft, dtivel, heng-liu, kartheekp-ms, nkolev92, rrelyea and zivkan as code owners August 14, 2020 21:45

advay26-zz requested a review from zkat August 14, 2020 23:18

loic-sharma reviewed Aug 15, 2020

View reviewed changes

advay26-zz changed the title ~~Basic functionality for Embedded Readmes~~ Basic pack functionality for Embedded Readmes Aug 17, 2020

zkat approved these changes Aug 18, 2020

View reviewed changes

nkolev92 reviewed Aug 19, 2020

View reviewed changes

Added dotnet pack tests

fde1434

nkolev92 approved these changes Aug 20, 2020

View reviewed changes

advay26-zz merged commit d03c785 into dev-feature-readme Aug 21, 2020

advay26-zz deleted the dev-advay26-readme branch August 21, 2020 21:18

campersau mentioned this pull request Aug 26, 2020

Support Embedded Readme NuGetPackageExplorer/NuGetPackageExplorer#1095

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic pack functionality for Embedded Readmes#3575

Basic pack functionality for Embedded Readmes#3575
advay26-zz merged 27 commits intodev-feature-readmefrom
dev-advay26-readme

advay26-zz commented Aug 12, 2020 •

edited

Loading

Uh oh!

zkat left a comment

Uh oh!

Uh oh!

loic-sharma Aug 15, 2020 •

edited

Loading

Uh oh!

advay26-zz Aug 17, 2020

Uh oh!

loic-sharma Aug 17, 2020 •

edited

Loading

Uh oh!

advay26-zz Aug 17, 2020

Uh oh!

nkolev92 left a comment

Uh oh!

advay26-zz commented Aug 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

advay26-zz commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Fix

Testing/Validation

Uh oh!

zkat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

loic-sharma Aug 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

advay26-zz Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

loic-sharma Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

advay26-zz Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

nkolev92 left a comment

Choose a reason for hiding this comment

Uh oh!

advay26-zz commented Aug 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

advay26-zz commented Aug 12, 2020 •

edited

Loading

loic-sharma Aug 15, 2020 •

edited

Loading

loic-sharma Aug 17, 2020 •

edited

Loading