Skip to content

Conversation

@chrisraygill
Copy link
Owner

@chrisraygill chrisraygill commented Sep 18, 2019

NuGetCleaner is meant to "clean up" the global package folder by deleting old or no longer relevant packages. The user specifies a max number of days since package "last access" (used in build) and anything outside of the range is deleted.

This PR is a way of letting people make comments on my code. The goal for me to have experienced developers give me feedback on everything from coding style and structure to choice of APIs and algorithms. I would like to ensure I become familiar with strong coding standards and habits for future projects. Any and all feedback is highly appreciated!

I imagine Program.cs and Option.cs should the subject of the most discussion since they contain my code, but if you find anything you wish to comment on on any of other files, feel free to do so!

Copy link

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

I'm sure you didn't mean to commit the nupkgs, so they should be removed. Consider adding the nupks directory to your .gitignore, or stop using the PackageOutputPath MSBuild property, so the nupkgs get created in a directory that is ignored.

}
}

public static int CheckDisableLastAccess()
Copy link

Choose a reason for hiding this comment

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

This method will only work on Windows, making the entire tool Windows only. If that's the intent, ok, but nothing in the package description makes that obvious, only if they look at the readme they might guess. So I can imagine people on Mac or Linux trying to install and use the tool.

You could work cross platform if you did something similar to the following:

  1. create an empty file (possibly need to contain at least 1 byte)
  2. use File.SetLastAccessFile to some time in the past (possibly need to read the value to make sure it's correct)
  3. read the file
  4. check the last access file, to make sure it's newer than the value you set in 2
  5. delete the file


int Days = options.Days;

if (options.DryRun)
Copy link

Choose a reason for hiding this comment

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

SearchAndPrint and SearchAndDestory are mostly the same, just the body of the inner most loop is different. I'd put that difference in individual methods and have a single Search method that accepts and calls an Action<string>. That reduces duplicate code.

@@ -0,0 +1,26 @@
# Your spec's journey
* Create a new wiki page using the spec template below, to jot your thoughts down.
* Create an [issue](https://github.com/NuGet/Home/issues) to track the feature or link an existing issue to the new wiki page. Engage the community on the feature. Feel free to tweet it from the @NuGet handle or ask the PM to tweet it out.
Copy link

Choose a reason for hiding this comment

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

This repo is not in the NuGet org, so I don't think it's appropriate to direct customers to file issues in NuGet/Home.

public static void Main(string[] args)
{
var osNameAndVersion = System.Runtime.InteropServices.RuntimeInformation.OSDescription;
if (!osNameAndVersion.Contains("Windows"))

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Consider:

private static IsWindows(){
            if (System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(System.Runtime.InteropServices.OSPlatform.Windows))
            {
                return true;
            }

            return false;
}

var osNameAndVersion = System.Runtime.InteropServices.RuntimeInformation.OSDescription;
if (!osNameAndVersion.Contains("Windows"))
{
Console.WriteLine("This tool is currently only available for Windows, sorry for the inconvenience!\n");

Choose a reason for hiding this comment

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

Explicit line endings are a bit strange. I would recommend either Environment.NewLine or another Console.WriteLine.

}

var settings = Settings.LoadDefaultSettings(".");
var gpfPath = SettingsUtility.GetGlobalPackagesFolder(settings);

Choose a reason for hiding this comment

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

👍


public static int CheckDisableLastAccess()
{
System.Diagnostics.Process process = new System.Diagnostics.Process();

Choose a reason for hiding this comment

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

dispose Process

return setting;
}

public static void SearchAndDestroy(string Path, int Days)

Choose a reason for hiding this comment

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

nice method name 😎

}
}

public static void RecursiveDelete(string dir)

Choose a reason for hiding this comment

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

`nugetcleaner` is available as a .NET Core Global Tool:

```bash
dotnet tool install --global NuGetCleaner --version 1.0.7

Choose a reason for hiding this comment

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

👍

<Description>Cleans up Global Package Folder by deleting directories unaccessed for a time greater than a specified threshold.</Description>
<PackageProjectUrl>https://github.com/chgill-MSFT/NuGetCleaner</PackageProjectUrl>
<RepositoryUrl>https://github.com/chgill-MSFT/NuGetCleaner</RepositoryUrl>
<GeneratePackageOnBuild>true</GeneratePackageOnBuild>

Choose a reason for hiding this comment

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

The .nupkg and .snupkg files should probably not be checked in to the repo.

Any .NET developer that installs packages (i.e. every .NET developer).

## Evidence
At the moment, my evidence is that Karan says he has seen complaints from customers about this issue. Exact evidence TBD.

Choose a reason for hiding this comment

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

Who is Karan? :) maybe mention his alias

Choose a reason for hiding this comment

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

Or better GitHub username


<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>netcoreapp2.1</TargetFramework>

Choose a reason for hiding this comment

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

You should multitarget 3.0

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants