Be more friendly to scripts #62
Conversation
This is beneficial for scripts that don't want to expose the password in the command line arguments.
|
Looks good, I think my only question is about the error messages as exceptions. Could we also attach a bool or int status return to those Tasks as well? I'd prefer not to have a stack trace for those common cases. |
DepotDownloader/Program.cs
Outdated
| password = Util.ReadPassword(); | ||
| Console.WriteLine(); | ||
| } | ||
| // Fallback to support redirected input |
There was a problem hiding this comment.
Why not just use Console.IsInputRedirected?
There was a problem hiding this comment.
I didn't know it existed, thanks! I didn't see your comments until I pushed a new revision below, but I'll look at it when I have time again.
DepotDownloader/ContentDownloader.cs
Outdated
| @@ -403,7 +403,7 @@ public static async Task DownloadAppAsync( uint appId, uint depotId, ulong manif | |||
| { | |||
| string contentName = GetAppOrDepotName( INVALID_DEPOT_ID, appId ); | |||
| Console.WriteLine( "App {0} ({1}) is not available from this account.", appId, contentName ); | |||
| return; | |||
| throw new Exception(String.Format("App {0} ({1}) is not available from this account.", appId, contentName)); | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I changed the throws to a custom exception now, in order to catch them but not interfere with unexpected and thus interesting exceptions and their tracebacks.
My original idea was to just raise unhandled exceptions for unhandled errors, letting someone else change it to a more specific exception if they wanted to catch it at a later time (this would be bad in a library, but I don't expect anyone to reuse DepotDownloader in that way). Other parts of the code seem to use Exception() for this already.
On a related note, do you know if the linter that gives this warning is available with the dotnet cli tools? I don't have a VisualStudio setup.
There was a problem hiding this comment.
It is, we just haven’t configured it (yet).
DepotDownloader/Program.cs
Outdated
| @@ -11,14 +11,14 @@ namespace DepotDownloader | |||
| class Program | |||
| { | |||
| static void Main( string[] args ) | |||
| => MainAsync( args ).GetAwaiter().GetResult(); | |||
| => System.Environment.Exit( MainAsync( args ).GetAwaiter().GetResult() ); | |||
There was a problem hiding this comment.
btw you can make the method signature int here and just return the int, there's no need to call Environment.Exit from the main function.
|
The revised exceptions look good. Console.IsInputRedirected should be investigated to see if it's implemented and working. |
When run by a script, the script needs to know if the requested operation was succesful. This patch makes sure error codes are returned for a number of unhandled error conditions.
|
Thanks for bearing with me, I pushed a new revision with the changes you requested. |
These changes make running DepotDownloader from a script less of a hassle.
I'm new to C#, so please carefully review the changes and bear with me if my solutions are not idiomatic. I'd love to get feedback!