Skip to content

Conversation

@pvdz
Copy link
Contributor

@pvdz pvdz commented Apr 28, 2025

Updated the CliJsonResult to CResult.

The type is a little less formal about the error value but allows for a message and cause now, leaving the data field open for a payload.

Also added code field to optionally be the exit code. The exit-points of the code, that print the response, are responsible for setting process.exitCode accordingly (defaulting to 1). This is because some errors are ignored so setting exitCode at point of error is not always desirable.

I believe I've covered all the commands. Updated types where necessary. All output points are setting the exit code. etc. Big PR, unfortunately.

  • Replace CliJsonResult with CResult
  • All fetches should return a CResult now, handlers should juggle them to the output files
  • All output files set exitCode according to the result
  • Eliminated most explicit throw statements in favor of CResult
  • Eliminated almost all the explicit exitCode setting outside of output files
  • All json output should go through serializeResultJson(), which will format it with two spaces and a trailing newline

The output files should now all expect a CResult and immediately set the exitCode accordingly. Then it should print the json or if it's an error, an error message. In some cases the json output is customized and there it will first handle the error case and print the json verbatim. Without json, they should print a nice error message if one happened.

There's still some considerations to be had around what to to when --json with --file is used; should errors be written to the file or terminal? I'm not sure, actually. Tempted to just remove the --file stuff in favor of having the user pipe output to a file instead. Arguably a DX regression but since the output should at this point be proper json when json is requested, is that a big deal?

There's a bit of followup to do for the API error handler function. I think we can tuck that into a CResult instead and simplify that part. But let's do that in a next PR.

@pvdz pvdz marked this pull request as ready for review April 29, 2025 10:55
@pvdz pvdz merged commit 401ff21 into main Apr 29, 2025
14 checks passed
@pvdz pvdz deleted the revisit_cresult branch April 29, 2025 13:25
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.

2 participants