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
Improve user feedback for cleanup node in installer #2207
Conversation
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 agree with this fix, however, I'm curious if we have any ability to give a more detailed error to the user, one that will let them know how many file(s) were not able to be processed?
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.
Upon further review detailed logging will be a bigger change. The more appropriate change here is to improve the behavior which this does so approving as is.
@donker please sign the CLA.
Ultimately the try-catch shouldn't be around the entire cleanup file as it is now. It should be on each line. But I didn't want to create any risk with this change of changing behavior. So left that as is for now. |
@@ -75,7 +75,7 @@ private bool ProcessCleanupFile() | |||
} | |||
catch (Exception ex) | |||
{ | |||
Log.AddFailure(ex); | |||
Log.AddWarning(string.Format(Util.CLEANUP_ProcessComplete, ex.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.
is this supposed to be using the CLEANUP_ProcessError
field you defined below?
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.
Yes. I'll adjust
Instead of showing an install as having an error, failing to clean up a file/folder should show a warning as it is not fatal to the installation of an extension.
Fix #2206