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

Change cmd setup to bubble up errors over exiting #454

Merged
merged 4 commits into from Jan 25, 2022

Conversation

mxygem
Copy link
Member

@mxygem mxygem commented Jan 25, 2022

Description

Based on some conversations around testing due to #383, I've done some refactoring of the cobra command section of the code to return errors to the caller and let cobra bubble the error up to the top level call to Execute.

Motivation & context

This change should allow the functions to be more easily testable instead of requiring potentially complicated test setup due to os.Exit() being called by the SUT.

Type of change

  • Refactoring/debt (improvement to code design or tooling without changing behaviour)

Checklist:

  • I have read the CONTRIBUTING document.
  • My change needed additional tests
    • I have added tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

@quine-bot
Copy link

quine-bot bot commented Jan 25, 2022

👋 Figuring out if a PR is useful is hard, hopefully this will help.

  • @mxygem has been on GitHub since 2016 and in that time has had 136 public PRs merged
  • Don't you recognize them? They've been here before 🎉
  • Here's a good example of their work: playtime (Supporting children and families experiencing homelessness in Washington, DC. Live app - https://wishlist.playtimeproject.org Organization Website:)
  • From looking at their profile, they seem to be good with Go and Shell.

Their most recently public accepted PR is: briandowns/spinner#125

@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #454 (19b29a8) into main (30de46d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #454   +/-   ##
=======================================
  Coverage   81.47%   81.47%           
=======================================
  Files          27       27           
  Lines        2186     2186           
=======================================
  Hits         1781     1781           
  Misses        312      312           
  Partials       93       93           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30de46d...19b29a8. Read the comment docs.

@mxygem mxygem added the 🏦 debt Tech debt label Jan 25, 2022
@mxygem mxygem requested a review from mattwynne January 25, 2022 19:18
@mxygem mxygem self-assigned this Jan 25, 2022
Copy link
Member

@vearutop vearutop left a comment

Choose a reason for hiding this comment

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

LGTM (apart from couple of minor nits)

@mxygem mxygem merged commit 5001c4f into main Jan 25, 2022
@mxygem mxygem deleted the update-builders-defer-to-main branch January 25, 2022 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏦 debt Tech debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants