Skip to content

Add details command#31743

Merged
maridematte merged 33 commits intodotnet:mainfrom
YuliiaKovalova:dev/ykovalova/add_details_command
Jun 28, 2023
Merged

Add details command#31743
maridematte merged 33 commits intodotnet:mainfrom
YuliiaKovalova:dev/ykovalova/add_details_command

Conversation

@YuliiaKovalova
Copy link
Copy Markdown
Member

@YuliiaKovalova YuliiaKovalova commented Apr 12, 2023

Implements the item: dotnet/templating#6683

Add details command to display more information about a template package.

@ghost ghost added the untriaged Request triage from a team member label Apr 12, 2023
@ghost
Copy link
Copy Markdown

ghost commented Apr 12, 2023

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@YuliiaKovalova
Copy link
Copy Markdown
Member Author

@vlada-shubina, please take a look when you come back.

<data name="Command_Details_Description" xml:space="preserve">
<value>
Provides details for specified Nuget package.
The command checks if the package is installed locally, if it wasn't found, then it searches on the https://www.nuget.org/.</value>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this respect Package Source Mapping, or does this just always use https://www.nuget.org/?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@maridematte/ @YuliiaKovalova for details we should be able to use any configured NuGet source, right? This doesn't require non-NuGet-API metadata like search does.

Separately, @KalleOlaviNiemitalo I'm not sure if the NuGet API exposes PSM data or if it's all implementation details. @YuliiaKovalova can you check with that team?

Comment thread src/Cli/Microsoft.TemplateEngine.Cli/TemplatePackageCoordinator.cs Outdated
Comment thread src/Cli/Microsoft.TemplateEngine.Cli/Commands/SymbolStrings.resx Outdated
@YuliiaKovalova
Copy link
Copy Markdown
Member Author

Current output:
image

Feedback to it:
my initial impression is that all of the required content is there, but it feels like a wall of text - I wonder if we could make the output look/feel more modern with use of spacing, coloration, weight, etc. There's also a bit of duplicated content - logically there are 6 templates in the package, but from the user perspective there are only 2 templates (that each support 3 languages) - can we represent that by collapsing/grouping the templates and showing a list in the Languages portion instead?
also if we have URLs to things, we should make those clickable in the terminal
and for things we don't have urls for (I'm specifically thinking of the NuGet license data where you can specify an SPDX license key like MIT GPLv3, etc) we should provide a link to the matching SPDX license URL. for example, MIT should link to https://spdx.org/licenses/MIT
MIT License | Software Package Data Exchange (SPDX)
(we can do this because SPDX is a well-known format that NuGet is standardized on, see the full list here: https://spdx.org/licenses/)
SPDX License List | Software Package Data Exchange (SPDX)

@KalleOlaviNiemitalo
Copy link
Copy Markdown
Contributor

KalleOlaviNiemitalo commented May 3, 2023

for example, MIT should link to https://spdx.org/licenses/MIT

OK, I suppose that is better than https://licenses.nuget.org/MIT, as spdx.org also hosts licenses like GFDL-1.2-invariants-only, which is not allowed on nuget.org but might be referenced by NuGet packages hosted elsewhere.

@baronfel
Copy link
Copy Markdown
Member

baronfel commented May 3, 2023

@KalleOlaviNiemitalo that's actually a great point - since NuGet.org is our source of truth here, we should use licenses.nuget.org instead of spdx. Have to bias towards consistency here.

@KalleOlaviNiemitalo
Copy link
Copy Markdown
Contributor

For accessibility, the text from the image in #31743 (comment):

PS C:\ykovalova_sdk\yk_fork_8\sdk> dotnet new details NUnit3.DotNetNew.Template --version "1.0.0"
   NUnit3.DotNetNew.Template
      Details:  Project and item templates containing basic NUnit 3 Test Project.
      Authors:  Aleksei Kharlov
      Owners:  akharlov, rprouse
      License Metadata:
         License Url:  https://github.com/halex2005/dotnet-new-nunit/blob/master/LICENSE
      Repository Url:  https://github.com/halex2005/dotnet-new-nunit
      Templates:
         NUnit 3 Test Project
            Short Names:  nunit
            Author:  Aleksei Kharlov aka halex2005 (codeofclimber.ru)
            Tags:  Test/NUnit
            Languages:  F#
            Details:  A project that contains NUnit tests that can run on .NET Core on Windows, Linux and macOS
         NUnit 3 Test Item
            Short Names:  nunit-test
            Author:  Aleksei Kharlov aka halex2005 (codeofclimber.ru)
            Tags: Test/NUnit
            Languages:  F#
            Details:  A item that contains NUnit tests
         NUnit 3 Test Project
            Short Names:  nunit
            Author:  Aleksei Kharlov aka halex2005 (codeofclimber.ru)
            Tags:  Test/NUnit
            Languages:  C#
            Details:  A project that contains NUnit tests that can run on .NET Core on Windows, Linux and macOS
         NUnit 3 Test Item
            Short Names:  nunit-test
            Author:  Aleksei Kharlov aka halex2005 (codeofclimber.ru)
            Tags: Test/NUnit
            Languages:  C#
            Details:  A item that contains NUnit tests
         NUnit 3 Test Project
            Short Names:  nunit
            Author:  Aleksei Kharlov aka halex2005 (codeofclimber.ru)
            Tags:  Test/NUnit
            Languages:  VB
            Details:  A project that contains NUnit tests that can run on .NET Core on Windows, Linux and macOS
         NUnit 3 Test Item
            Short Names:  nunit-test
            Author:  Aleksei Kharlov aka halex2005 (codeofclimber.ru)
            Tags: Test/NUnit
            Languages:  VB
            Details:  A item that contains NUnit tests
PS C:\ykovalova_sdk\yk_fork_8\sdk>

@KalleOlaviNiemitalo
Copy link
Copy Markdown
Contributor

https://github.com/NuGet/Home/wiki/Packaging-License-within-the-nupkg-(Technical-Spec)#approach-for-in-house-packages-%2D-unlicensed suggests UNLICENSED as the license expression for in-house packages that should not be posted to public feeds. Neither https://spdx.org/licenses/UNLICENSED nor https://licenses.nuget.org/UNLICENSED explains that meaning. I think dotnet details should recognize UNLICENSED and not translate it to a useless URL.

@baronfel
Copy link
Copy Markdown
Member

baronfel commented May 3, 2023

Here is my sketched up feedback for the display output - note the # are comments on each line of output:

PS C:\ykovalova_sdk\yk_fork_8\sdk> dotnet new details NUnit3.DotNetNew.Template --version "1.0.0"
   NUnit3.DotNetNew.Template
      Details:  Project and item templates containing basic NUnit 3 Test Project.
      Authors:  Aleksei Kharlov
      Owners:  akharlov, rprouse # each of these should link to the owners page on nuget.org (e.g. https://www.nuget.org/profiles/akharlov)
      License Metadata: # depending on how the license is provided in the package, this should change
         License Url:  https://github.com/halex2005/dotnet-new-nunit/blob/master/LICENSE # clickable link
         License Expression: MIT # link to licenses.nuget.org/MIT per https://learn.microsoft.com/en-us/nuget/nuget-org/licenses.nuget.org#request
      Repository Url:  https://github.com/halex2005/dotnet-new-nunit # clickable link
      Templates:
         NUnit 3 Test Project
            Short Names:  nunit
            Author:  Aleksei Kharlov aka halex2005 (codeofclimber.ru)
            Tags:  Test/NUnit # tags should be split on '/' and each tag should be a clickable link to the NuGet.org query for that tag: https://www.nuget.org/packages?q=Tags%3A%22Test%22
            Languages:  C#, F#, VB # group the languages available for each template
            Details:  A project that contains NUnit tests that can run on .NET Core on Windows, Linux and macOS
         NUnit 3 Test Item
            Short Names:  nunit-test
            Author:  Aleksei Kharlov aka halex2005 (codeofclimber.ru)
            Tags: Test/NUnit
            Languages:  C#, F#, VB
            Details:  A item that contains NUnit tests

@KalleOlaviNiemitalo
Copy link
Copy Markdown
Contributor

also if we have URLs to things, we should make those clickable in the terminal

Windows Terminal already recognizes URLs in the output and makes them clickable, without needing .NET SDK to mark them specially.
I don't know whether there are terminals that support hyperlinks but do not recognize URLs automatically. (There certainly are terminals that do not support hyperlinks at all.)
If you wanted to display some other text but make it a hyperlink to a URL (e.g. display a license expression and link to the license text), then you'd have to explicitly output OSC 8 to mark the hyperlink. https://github.com/Alhadis/OSC8-Adoption/

@baronfel
Copy link
Copy Markdown
Member

baronfel commented May 3, 2023

@KalleOlaviNiemitalo yes, all feedback about clickable links is predicated on the terminal supporting control codes to signal that. This is a muscle that the CLI and MSBuild are learning this release cycle and we're fairly confident it in - though of course there should always be an explicit opt out.

@KalleOlaviNiemitalo
Copy link
Copy Markdown
Contributor

If a package has an embedded license file, how will the command display that?

@baronfel
Copy link
Copy Markdown
Member

baronfel commented May 3, 2023

Might just have to be a file:// URI to the nuspec and let it open with whatever file handler the users system has configured for nuspec files.

@maridematte
Copy link
Copy Markdown
Member

The current display of details looks like this:

image

I added the table to not feel like a wall of text.

@maridematte
Copy link
Copy Markdown
Member

This is the text from the above image:

NUnit3.DotNetNew.Template
   Details:  Project and item templates containing basic NUnit 3 Test Project.
   Authors:
      Aleksei Kharlov
   Owners:
      https://nuget.org/profiles/akharlov
      https://nuget.org/profiles/rprouse
   License Metadata:
      License Url: https://github.com/halex2005/dotnet-new-nunit/blob/master/LICENSE
      Repository Url: https://github.com/halex2005/dotnet-new-nunit

   Templates:
      Template Name         Short Name  Type     Tags        Language
      --------------------  ----------  -------  ----------  --------
      NUnit 3 Test Project  nunit       project  Test/NUnit  C#,F#,VB
      NUnit 3 Test Item     nunit-test  item     Test/NUnit  C#,F#,VB 

@baronfel
Copy link
Copy Markdown
Member

I like this a lot @maridematte. Will we be able to make those URLs clickable? That's the only major usability feature I see missing. Happy to see this getting close!

@maridematte
Copy link
Copy Markdown
Member

The urls are clickable currently, if you're using windows terminal you can press ctrl + left click to navigate to the url.

Comment thread src/Cli/Microsoft.TemplateEngine.Cli/TemplatePackageCoordinator.cs Outdated
@maridematte maridematte marked this pull request as ready for review June 20, 2023 13:20
@maridematte maridematte requested a review from a team as a code owner June 20, 2023 13:20
@YuliiaKovalova
Copy link
Copy Markdown
Member Author

Looks good to me!

Comment thread src/Tests/dotnet-new.Tests/DotnetNewDetailsTest.cs Outdated
@YuliiaKovalova
Copy link
Copy Markdown
Member Author

@maridematte, I would recommend adding in output 3 more fields: Version, Source Feed, and Prefix Reserved flag.

@vlada-shubina
Copy link
Copy Markdown
Member

The current display of details looks like this:

image I added the table to not feel like a wall of text.

this output misses:

  • version of the package
  • NuGet feed the information is taken from
  • indication if prefix is reserved

@vlada-shubina
Copy link
Copy Markdown
Member

Tests: i would recommend to cover at least the following scenarios

  • the package is not installed
    • the package is from NuGet.org (with / without version, with templates information)
    • the package is from other feed (with / without version, without package information0
  • the package is installed
    • the package is from NuGet.org
    • the package is from other feed
    • the package is installed from local *.nupkg
    • the package is the folder

Copy link
Copy Markdown
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

There are a few notes about localization of user-facing strings and a couple layout notes that we spoke about offline as well. Overall I like the state of this a lot!

Comment thread src/Cli/Microsoft.TemplateEngine.Cli/TemplatePackageCoordinator.cs Outdated
Comment thread src/Cli/Microsoft.TemplateEngine.Cli/TemplatePackageCoordinator.cs Outdated
Comment thread src/Cli/Microsoft.TemplateEngine.Cli/TemplatePackageCoordinator.cs Outdated
Comment thread src/Cli/Microsoft.TemplateEngine.Cli/TemplatePackageCoordinator.cs Outdated
Comment thread src/Cli/Microsoft.TemplateEngine.Cli/TemplatePackageCoordinator.cs Outdated
Comment thread src/Cli/Microsoft.TemplateEngine.Cli/TemplatePackageCoordinator.cs Outdated
Comment on lines +1083 to +1098
private IEnumerable<PackageSource> LoadNuGetSources(IEnumerable<string>? additionalSources, bool includeNuGetFeed)
{
IEnumerable<PackageSource> defaultSources;
string currentDirectory = string.Empty;
try
{
currentDirectory = Directory.GetCurrentDirectory();
ISettings settings = global::NuGet.Configuration.Settings.LoadDefaultSettings(currentDirectory);
PackageSourceProvider packageSourceProvider = new PackageSourceProvider(settings);
defaultSources = packageSourceProvider.LoadPackageSources().Where(source => source.IsEnabled);
if (includeNuGetFeed)
{
var nuGetFeed = new PackageSource(NugetOrgFeed);
defaultSources = defaultSources.Append(nuGetFeed);
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Future expansion: I think if string packageIdentity was added to this method, then we could pretty easily add support for PackageSourceMapping to this feature via something like this:

PackageSourceMapping packageSourceMapping = PackageSourceMapping.GetPackageSourceMapping(settings);
IReadOnlyList<string> validPackageSources = packageSourceMapping.GetConfiguredPackageSources(packageIdentity);
defaultSources = packageSourceProvider.LoadPackageSources().Where(source => source.IsEnabled && (validPackageSources?.Contains(source.Name) ?? true));

We should consider PSM support all-up for templating, though.

Comment thread src/Cli/Microsoft.TemplateEngine.Cli/TemplatePackageCoordinator.cs Outdated
@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/add_details_command branch from 2385aa9 to ee0a4e4 Compare June 27, 2023 20:28
@maridematte maridematte merged commit 282088f into dotnet:main Jun 28, 2023
@YuliiaKovalova
Copy link
Copy Markdown
Member Author

YuliiaKovalova commented Jun 28, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

untriaged Request triage from a team member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants