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

CLI JSON updates #962

Merged
merged 11 commits into from
Feb 25, 2021
Merged

CLI JSON updates #962

merged 11 commits into from
Feb 25, 2021

Conversation

jefferai
Copy link
Member

NOTE: Before I merge this I will separate it into two separate PRs -- one that updates the api bits (and then tag it), and another that then uses that new API tag with the main repo bits. I will also add the changelog notices then as well.

This makes the following changes:

  • In JSON-formatted CLI output, an individual item is under the item key and a list is under the items key. Additionally, the status code is returned in the top level object as status_code, because (a) why not and (b) it gives a clear (and potentially useful) example for why we're making this change
  • In JSON-formatted CLI output, if an error is printed, it now prints as an object with an error key.
  • In JSON-formatted CLI output, if an API error is printed, it now prints as an object with CLI context in the error key, a status code, and the raw API error itself in the api_error key.
  • I removed the ResponseBody and ResponseMap functions in favor of simply Response. We didn't want to do this until we felt like there was a reason to do so, and I could have worked around doing this here, but it feels like once we get to three or so helper functions just to not return the actual Response, let's just return the response.
  • I renamed ResponseStatus to StatusCode. Given that our errors have Kind, Op, and Message, and those are all strings, neither Status nor Code conflicts; meanwhile this is the name that is used in Go's http.Response so it felt more canonical.

louisruch
louisruch previously approved these changes Feb 24, 2021
Copy link
Collaborator

@louisruch louisruch left a comment

Choose a reason for hiding this comment

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

Looks great!

talanknight
talanknight previously approved these changes Feb 24, 2021
Copy link
Contributor

@talanknight talanknight left a comment

Choose a reason for hiding this comment

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

These changes look good.

I am a little bummed (but only a little) that we are deciding to expose the underlying response instead of targeting more precisely the data we want to expose since, in my mind, the fact we use http is an implementation detail which could be hidden.

@ghost
Copy link

ghost commented Feb 24, 2021

@jefferai one change I noticed is that errors are now printed to stdout rather than stderr. Is this intentional?

@jefferai
Copy link
Member Author

Errors back on stderr as discussed.

talanknight
talanknight previously approved these changes Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants