-
Notifications
You must be signed in to change notification settings - Fork 85
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 Installer Exceptions for handling matching exceptions from core library #138
Conversation
{ | ||
Logger.ErrorLocalized(nameof(Resources.NewInstallerUrlMustMatchExisting_Message)); | ||
DisplayMismatchedArchitectures(detectedArchOfInstallers); | ||
Logger.ErrorLocalized(nameof(Resources.PackageParsing_Error)); |
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.
This still looks like some errrors are return via exceptions, and others via exit code. Why do we have split error handling? #Resolved
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 believe in this case, all our errors should be handled with exceptions. That way we can obtain better information about the specific packages that failed parsing. I modified the method so that there's no split error handling
@@ -288,6 +288,12 @@ protected static async Task<string> DownloadPackageFile(string installerUrl) | |||
return null; | |||
} | |||
|
|||
if (e is InvalidDataException) | |||
{ | |||
Logger.ErrorLocalized(nameof(Resources.DownloadFileExceedsMaxSize_Error), e.Message); |
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.
Instead of just putting this in the message i rather like making these strange typed values on the exception, like what you did for the other exception. In that manner, we can still jhave a meaningful message on the exception for debugging, but the data the caller needs to derive a good localized error message can still be handed out? #Resolved
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 created a new exception for DownloadSizeExceeded similar to the other exceptions I created so that the data value isn't hidden in the exception message.
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.
Lol.. "Strong" not strange :)
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.
Resolves #101
Instead of adding localization to the core library as a means for conveying an error to the calling function, I believe our issues can be fixed by throwing custom exceptions and catching those thrown exceptions in the calling function. As of right now, there hasn't been a need to add any logging functionality to the core library as all of the logging takes place in the CLI and we should try to maintain that design choice until we absolutely need to make that change.
Changes:
Microsoft Reviewers: Open in CodeFlow