Easily publish local packages #12
Conversation
Summary of ChangesHello @PieterjanDeClippel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the 'Dev' tool by adding a new 'pack' command, which significantly simplifies the process of publishing local NuGet packages. Instead of manually specifying the output directory, the tool now intelligently parses the user's 'nuget.config' to determine the correct package source path, thereby streamlining the development workflow for package creation and distribution. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new pack command to easily publish local NuGet packages by looking up a source name in the user's nuget.config. The implementation is a good start, but has a few critical issues related to error handling and argument validation that could cause the application to crash. I've provided suggestions to make the new command more robust by adding checks for missing arguments and handling cases where configuration files or package sources are not found. I also included a suggestion to improve code style and pointed out an opportunity to extend the pack command's functionality to project files for consistency.
| if (command is "pack") | ||
| { | ||
| AnsiConsole.MarkupLine($"[green]Packing NuGet packages for[/] {slnFile} [green]...[/]"); | ||
| var nugetConfigPath = GetRootNugetConfigPath(); | ||
| var nugetConfig = XDocument.Load(nugetConfigPath); | ||
| var sourceName = commandArgs[0]; | ||
| var nugetSourcePath = nugetConfig.Element("configuration")?.Element("packageSources")?.Elements("add")? | ||
| .Select(x => new | ||
| { | ||
| Name = x.Attribute("key")?.Value, | ||
| Source = x.Attribute("value")?.Value | ||
| })? | ||
| .FirstOrDefault(x => x.Name?.Equals(sourceName, StringComparison.InvariantCultureIgnoreCase) ?? false)? | ||
| .Source; | ||
|
|
||
| Process.Start("dotnet", $"pack \"{slnFile}\" -c Release -o \"{nugetSourcePath}\"").WaitForExit(); | ||
| AnsiConsole.MarkupLine($"[green]NuGet packages for[/] {slnFile} published to {nugetSourcePath}[green][/]"); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This new pack command logic has a few issues that could lead to crashes or unexpected behavior:
- Missing Argument Check: It directly accesses
commandArgs[0]without verifying that an argument was provided. This will cause anIndexOutOfRangeExceptionifdev packis run without a source name. - Unhandled Nulls: If the NuGet source isn't found,
nugetSourcePathbecomesnull, which is then passed todotnet pack. This should be handled with a clear error message. - Inefficient LINQ: The LINQ query can be simplified to avoid creating an unnecessary anonymous object for each package source.
The suggested change makes this logic more robust by adding validation and improving the query. This suggestion assumes GetRootNugetConfigPath is modified to return null on failure, as suggested in another comment.
if (command is "pack")
{
if (commandArgs.Length == 0)
{
AnsiConsole.MarkupLine("[red]Error: Missing package source name for 'pack' command.[/]");
AnsiConsole.MarkupLine("[dim]Usage: dev pack <source-name>[/]");
return false;
}
var sourceName = commandArgs[0];
AnsiConsole.MarkupLine($"[green]Packing NuGet packages for[/] {slnFile} [green]...[/]");
var nugetConfigPath = GetRootNugetConfigPath();
if (nugetConfigPath is null)
{
AnsiConsole.MarkupLine("[red]Could not find user NuGet configuration file.[/]");
return false;
}
var nugetConfig = XDocument.Load(nugetConfigPath);
var nugetSourcePath = nugetConfig.Element("configuration")?
.Element("packageSources")?
.Elements("add")
.FirstOrDefault(x => string.Equals(x.Attribute("key")?.Value, sourceName, StringComparison.InvariantCultureIgnoreCase))?
.Attribute("value")?.Value;
if (string.IsNullOrEmpty(nugetSourcePath))
{
AnsiConsole.MarkupLine($"[red]Error: NuGet source '{sourceName}' not found in your nuget.config.[/]");
return false;
}
Process.Start("dotnet", $"pack \"{slnFile}\" -c Release -o \"{nugetSourcePath}\"").WaitForExit();
AnsiConsole.MarkupLine($"[green]NuGet packages for[/] {slnFile} published to {nugetSourcePath}[green][/]");
return true;
}| static string GetRootNugetConfigPath() | ||
| { | ||
| var userConfig = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), "NuGet", "nuget.config"); | ||
| var linuxMacConfig = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".nuget", "NuGet", "NuGet.Config"); | ||
|
|
||
| var configPath = File.Exists(userConfig) ? userConfig | ||
| : File.Exists(linuxMacConfig) ? linuxMacConfig | ||
| : throw new FileNotFoundException("Could not find user NuGet configuration file."); | ||
|
|
||
| return configPath; | ||
| } |
There was a problem hiding this comment.
This method throws a FileNotFoundException which is not handled by the caller, causing the application to crash. A better approach for this helper method would be to return null if the file is not found and let the caller handle the null case and display an appropriate error message. This makes the method more reusable and the control flow clearer.
static string? GetRootNugetConfigPath()
{
var userConfig = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), "NuGet", "nuget.config");
if (File.Exists(userConfig))
{
return userConfig;
}
var linuxMacConfig = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".nuget", "NuGet", "NuGet.Config");
if (File.Exists(linuxMacConfig))
{
return linuxMacConfig;
}
return null;
}| using Dev; | ||
| using Microsoft.VisualStudio.SolutionPersistence.Model; | ||
| using Microsoft.VisualStudio.SolutionPersistence.Serializer; | ||
| using Spectre.Console; | ||
| using System.Diagnostics; | ||
| using System.Runtime.InteropServices; | ||
| using System.Text.Json; | ||
| using System.Text.RegularExpressions; | ||
| using System.Xml.Linq; |
There was a problem hiding this comment.
For consistency and adherence to common C# style guidelines, it's best to sort using directives. The standard convention is to place System namespaces first, followed by other namespaces, with each group sorted alphabetically.
using System.Diagnostics;
using System.Runtime.InteropServices;
using System.Text.Json;
using System.Text.RegularExpressions;
using System.Xml.Linq;
using Dev;
using Microsoft.VisualStudio.SolutionPersistence.Model;
using Microsoft.VisualStudio.SolutionPersistence.Serializer;
using Spectre.Console;| Process.Start("dotnet", $"pack \"{slnFile}\" -c Release -o \"{nugetSourcePath}\"").WaitForExit(); | ||
| AnsiConsole.MarkupLine($"[green]NuGet packages for[/] {slnFile} published to {nugetSourcePath}[green][/]"); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The pack command is currently only implemented for solution files (.sln). For consistency with other commands like build and bump, consider adding support for packing individual project files (.csproj) as well.
1 participant
Description
We can use
dotnet pack -o C:\bliblablue\NugetPackages, but that's cumbersome.By translating a source-name to path first we can make it easier. We just have to look it up in our
%userprofile%nuget.configUsage
Where
Localis the name of a PackageSourceRelated issues
Screenshots
Tested on a local solution