-
Notifications
You must be signed in to change notification settings - Fork 224
Conversation
Hi @muratg, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
{ | ||
public Reports Reports { get; } | ||
public string HttpCacheDirectory { get; } | ||
public ClearCacheCommand(Reports reports, string httpCacheDirectory) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix all the formatting please 😄 Empty line above the ctor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
{ | ||
public static void Register(CommandLineApplication cmdApp, ReportsFactory reportsFactory) | ||
{ | ||
cmdApp.Command("clearcache", c => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clear-cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 looks like we hyphenate multi-word commands in dnu in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually :-1/2: because it looks like we randomly do things in random places 😄
-s|--source <FEED>
-f|--fallbacksource <FEED>
-p|--proxy <ADDRESS>
--no-cache
--packages
--ignore-failed-sources
--quiet
--parallel
-?|-h|--help
--fallbacksource
is one word, but --ignore-failed-sources
is multi-word.
Do whatever you want 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer hyphenating. If we need to change other things to be consistent (eventually) so be it.
} | ||
public void DeleteDirectoryRecursively(string baseDirectory, bool deleteBaseDirectory) | ||
{ | ||
var files = Directory.GetFiles(baseDirectory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about System.IO.Directory.Delete(string path, bool recursive)
? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be safe to delete the base directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, restore
creates it if it doesn't exist. I was thinking to keep it safe if we change the restore behavior in the future though.
But fair enough, I'll make the change to simplify the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using the helper that already exists 😄 https://github.com/aspnet/dnx/blob/dev/src/Microsoft.Dnx.Tooling/Utils/FileOperationUtils.cs#L20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we wrote a helper to do something the BCL already does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/aspnet/dnx/blob/dev/src/Microsoft.Dnx.Tooling/Utils/FileOperationUtils.cs#L15
There used to be logic that marked readonly files as not readonly I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, will switch to the helper. As dnu restore
creates the cache directory if it doesn't exist, it is safe to do.
try | ||
{ | ||
FileOperationUtils.DeleteFolder(HttpCacheDirectory); | ||
Reports.Information.WriteLine("Cache cleared."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have some extension methods for Reports
to write information directly.
Fixed: 3d0edc0 |
Implements #2401