Skip to content
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

Clean up some error messages #2032

Merged
merged 1 commit into from
Mar 12, 2018
Merged

Clean up some error messages #2032

merged 1 commit into from
Mar 12, 2018

Conversation

stkb
Copy link
Contributor

@stkb stkb commented Feb 20, 2018

Cleans up code that outputs error messages:

  • Removes the extra 'ERROR' text in the case of error 'ERROR...
  • Uses the error function instead in some cases of write-host '...' -f Red/DarkRed...
  • ...Except when an exit comes after it, in which case it's changed to use abort (in some cases this changes the text from darkred to red). The abort method doesn't prepend any text.

Haven't touched these cases:

  • Code that (effectively) uses write-output instead of write-host to output error messages.
  • Code that uses [console]::error.writeline instead, since I don't know exactly what this does/why it was used
  • Lots of code uses write-host -f DarkRed "...", but I haven't changed them all to use the error function because I don't know if preceding every error message with "ERROR " was really desired.

Additional note: For the scoop commands there's some inconsistency in what they do when arguments are missing. Some use error (in turn using write-host), eg:
https://github.com/lukesampson/scoop/blob/35eb73b2dbc4a8cde6189c8b46eccf03113b9f3b/libexec/scoop-install.ps1#L62

Others just output the text, eg:
https://github.com/lukesampson/scoop/blob/35eb73b2dbc4a8cde6189c8b46eccf03113b9f3b/libexec/scoop-cache.ps1#L41

Removes extra 'ERROR' in text where the `error` method is used.
Converts some `write-host "error message"` to use the `error` method
Converts some `write-host "error message"; exit 1` to use the `abort`
method.
@@ -65,7 +65,7 @@ $queue | % {

if ($json.checkver -eq "github") {
if (!$json.homepage.StartsWith("https://github.com/")) {
write-host "ERROR: $name checkver expects the homepage to be a github repository" -f DarkYellow
error "$name checkver expects the homepage to be a github repository"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though the original was darkyellow text, changed to error instead of warn, since it seemed like an error condition.

@r15ch13
Copy link
Member

r15ch13 commented Feb 20, 2018

Regarding the scoop-cache.ps1 it's clearly an error message so adding the error function is reasonable.
auto-pr.ps1 and checkver.ps1 are mainly used for maintenance, so they aren't that important 😁

PS: there goes my plan to remove the abort function 😭😁

@stkb
Copy link
Contributor Author

stkb commented Feb 20, 2018

PS: there goes my plan to remove the abort function 😭😁

If you want to go another way, it's fine by me :)

@r15ch13
Copy link
Member

r15ch13 commented Feb 20, 2018

The complete error handling of scoop has to be changed in order to remove the abort function. So it's still okay to use it 😄


$architecture = default_architecture
try {
$architecture = ensure_architecture ($opt.a + $opt.arch)
} catch {
error "ERROR: $_"; exit 1
abort "ERROR: $_"
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but kinda confusing. Could abort either not prefix 'ERROR: ', or use a different prefix, like 'FATAL: '?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could, but I wouldn't worry too much about it. As @r15ch13 said, the whole error handling needs to be changed anyway. The worst cases ("ERROR ERROR...") have been taken care of.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. thx.

@stkb
Copy link
Contributor Author

stkb commented Mar 9, 2018

Having thought about it some more, yeah this should all really be done a different way.

IMO most inner functions shouldn't be writing error messages, or possibly anything to the screen directly, but rather returning a success value or throwing errors if something goes wrong. Only at the last moment should the result be translated into success or error messages on-screen. This makes it much easier to test and to know what's going on in the code.

@r15ch13 r15ch13 merged commit 31075f6 into ScoopInstaller:master Mar 12, 2018
@stkb stkb deleted the errors branch March 12, 2018 19:08
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.

3 participants