Skip to content

Conversation

cnsnyder
Copy link

This PR adds checks for the errors potentially returned in a number of places as documented here : #45

This code is not as DRY as I'd like it, all feedback welcome! I hope to solve issues and improve!

Cheers!

@Lorquas
Copy link
Member

Lorquas commented Feb 25, 2025

Can one of the admins verify this patch?

Copy link
Contributor

@subpop subpop left a comment

Choose a reason for hiding this comment

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

LGTM

@subpop
Copy link
Contributor

subpop commented Feb 27, 2025

This code is not as DRY as I'd like it, all feedback welcome! I hope to solve issues and improve!

I think there's a follow-up commit or PR we can introduce here that changes the behavior of the showErrorMessages function to return a normal error, instead of a bunch of empty cli.Exit() errors. Then we update the callers of showErrorMessage to check for the returned error and wrap it in a cli.Exit.

Copy link

@DuckBoss DuckBoss left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes.

I had the same thoughts as Link when I saw the showErrorMessages method using cli.Exit(), and I would prefer the logic changes to be in a separate small PR instead of this one.

Copy link
Contributor

@subpop subpop left a comment

Choose a reason for hiding this comment

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

The commit linter is failing. 650578b contains a capitalized "Fix". It should be lower-case to conform to Conventional Commits. Do you mind amending the commit and changing the message?

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.

4 participants