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 nuget package status indicator proof-of-concept #8539

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

adamint
Copy link
Member

@adamint adamint commented Sep 27, 2022

This proof-of-concept obtains package vulnerability/updatable/deprecation info from the dotnet cli.
image

vulnerable = excel
deprecated = sharepoint
upgrade available = word

Issues:

  • Source refreshes every 15 minutes, but won't be reflected until a nuget restore is done
  • Currently, retrieval is synchronous the first time
  • We need dedicated icons

Resolves #8136

Microsoft Reviewers: Open in CodeFlow

@adamint adamint added the Feature-Dependency-Node "Dependencies" node in Solution Explorer that display project, binary & package references label Sep 27, 2022
base.Initialize();
#pragma warning restore RS0030

_broadcastBlock = DataflowBlockSlim.CreateBroadcastBlock<IProjectVersionedValue<Dictionary<string, DiagnosticLevel>>>(nameFormat: $"{nameof(DependencyNugetUpdateBlock)} {1}");
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
_broadcastBlock = DataflowBlockSlim.CreateBroadcastBlock<IProjectVersionedValue<Dictionary<string, DiagnosticLevel>>>(nameFormat: $"{nameof(DependencyNugetUpdateBlock)} {1}");
_broadcastBlock = DataflowBlockSlim.CreateBroadcastBlock<IProjectVersionedValue<Dictionary<string, DiagnosticLevel>>>(nameFormat: $"{nameof(DependencyNugetUpdateBlock)} {{1}}");

namespace Microsoft.VisualStudio.ProjectSystem.Tree.Dependencies;

[Export(typeof(IDependencyNugetUpdateBlock))]
internal class DependencyNugetUpdateBlock: ProjectValueDataSourceBase<Dictionary<string, DiagnosticLevel>>, IDependencyNugetUpdateBlock
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
internal class DependencyNugetUpdateBlock: ProjectValueDataSourceBase<Dictionary<string, DiagnosticLevel>>, IDependencyNugetUpdateBlock
internal class DependencyNuGetUpdateBlock : ProjectValueDataSourceBase<Dictionary<string, DiagnosticLevel>>, IDependencyNugetUpdateBlock

Comment on lines +69 to +73
nameof(DiagnosticLevel.Warning) => DiagnosticLevel.Warning,
nameof(DiagnosticLevel.Error) => DiagnosticLevel.Error,
nameof(DiagnosticLevel.UpgradeAvailable) => DiagnosticLevel.UpgradeAvailable,
nameof(DiagnosticLevel.Deprecation) => DiagnosticLevel.Deprecation,
nameof(DiagnosticLevel.Vulnerability) => DiagnosticLevel.Vulnerability,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about replacing the string literals with nameof. The strings are defined in MSBuild data. The C# enum is not explicitly tied to that. In some cases we have XAML rules with generated classes that provide constants for these values, but that's not the case here.

Consider in future that someone renames an enum member. It's not obvious that such a change would break our binding to the MSBuild item metadata contract.

Comment on lines +140 to +154
switch (DiagnosticLevel)
{
case DiagnosticLevel.UpgradeAvailable:
return KnownMonikers.OfficeWord2013;
case DiagnosticLevel.Warning:
return IconSet.UnresolvedIcon;
case DiagnosticLevel.Deprecation:
return KnownMonikers.OfficeSharePoint2013;
case DiagnosticLevel.Error:
return IconSet.UnresolvedIcon;
case DiagnosticLevel.Vulnerability:
return KnownMonikers.OfficeExcel2013;
default:
throw new ArgumentOutOfRangeException();
}
Copy link
Member

Choose a reason for hiding this comment

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

To be clear (I know this is a draft) but we should consider adding unresolved/deprecation/vulnerability icon members to IconSet. The Dependency class applies to all kinds of dependencies, not just packages. If another dependency type used one of these new diagnostic levels, we'd end up showing a NuGet package icon for it if we hard coded them here.

This comment is based on the assumption that we end up with package reference icons with new overlays for the three new states.

For non-package dependencies we would likely re-use the warning/error icons for those diagnostic levels, as we don't expect to display them and therefore wouldn't ask the design team to produce such icons for us.

Brief history detour: These icons used to separate the base icon from the overlay. However in 17.0 the design team wanted finer control over how the overlay appeared, and so baked a flattened form of each combination into its own icon.

Copy link
Member Author

Choose a reason for hiding this comment

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

@drewnoakes so what exactly would be requested to be created of the design team? Just the overlays with no nuget package icon?

Copy link
Member

Choose a reason for hiding this comment

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

For each new overlay, you'd need a version of it displayed on top of the NuGet icon. That is, baked into a single image for each overlay.

namespace Microsoft.VisualStudio.ProjectSystem.Tree;

[ProjectSystemContract(ProjectSystemContractScope.UnconfiguredProject, ProjectSystemContractProvider.Private, Cardinality = ImportCardinality.ExactlyOne)]
internal interface IDependencyNugetUpdateBlock : IProjectValueDataSource<Dictionary<string, DiagnosticLevel>>
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
internal interface IDependencyNugetUpdateBlock : IProjectValueDataSource<Dictionary<string, DiagnosticLevel>>
internal interface IDependencyNuGetUpdateBlock : IProjectValueDataSource<Dictionary<string, DiagnosticLevel>>

@MiYanni
Copy link
Member

MiYanni commented Feb 17, 2023

@adamint Howdy. The related issue to this was closed. Would it make sense to close this PR and reopen it if the related issue is reopened?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature-Dependency-Node "Dependencies" node in Solution Explorer that display project, binary & package references
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nuget Package Status Indicators - Proof of Concept
3 participants