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

Make JSON printing a bit nicer #193

Merged
merged 1 commit into from
Apr 5, 2016
Merged

Make JSON printing a bit nicer #193

merged 1 commit into from
Apr 5, 2016

Conversation

gdb
Copy link
Contributor

@gdb gdb commented Mar 24, 2016

This change makes JSON generation more consistent and human-friendly:

  • Adding a trailing newline is nicer to people using curl from the command line, without making the machine case worse.
  • Add indent=2 to errors so they are pretty printed as well.

@@ -46,7 +46,7 @@ def problem(status, title, detail, type='about:blank', instance=None, headers=No
if ext:
problem_response.update(ext)

body = json.dumps(problem_response)
body = json.dumps(problem_response, indent=2) + "\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

The + "\n" means you create 3 string every time instead of 1.

@jmcs
Copy link
Contributor

jmcs commented Mar 24, 2016

The unit tests are also failing.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.432% when pulling ea3b2c9 on gdb:master into 5e5d97e on zalando:master.

@gdb
Copy link
Contributor Author

gdb commented Mar 24, 2016

Whoops, sorry yes I'd meant to ask about whether the implementation and functionality was ok before fixing up the tests. (On the one hand, copies always sound bad. On the other hand, there was just a json.dumps there, which I'd expect would always be much more expensive than the copy. You also mention 3 strings — are you concerned about creating the '\n'?)

I just switched the implementation to not do the copy (but instead pass a list to the Response — I'm less familiar with the Python WSGI ecosystem than the Ruby Rack one, but I believe this should be supported regardless of response_class?), and fixed the tests — is this any better?

@ainmosni
Copy link
Contributor

Is it really the frameworks responsibility to pretty print it? I mean, everyone working with REST has tools to pretty print it on their end. At the very least they can pipe it to python -m json.tool or jq.

@gdb
Copy link
Contributor Author

gdb commented Mar 24, 2016

At Stripe, one of our core observations was that the little details are what changes an API from acceptable to actually a pleasure to use. (And particularly, the most important details happen during interactive development: when you're first curling around a new API. We would do things like pre-fill your test API key in the API docs, so you can just copy-paste the examples directly without having to figure out where your API key lives, etc.)

Certainly developers can pipe the output of a curl to jq. But even better would be anticipating this use-case and returning the JSON already in a human-friendly format.

Regardless of the philosophy, this framework already does indent=2 in one case; here I've made it consistent and added trailing newlines. I'd also be equally happy if this functionality were easily pluggable. But I think it's actually a pretty good thing for the framework to provide little finishing touches that otherwise most people probably wouldn't bother with.

@hjacobs
Copy link
Contributor

hjacobs commented Mar 27, 2016

@gdb maybe you can consider using HTTPie, https://github.com/jkbrzt/httpie

.. user-friendly curl replacement with intuitive UI, JSON support, syntax highlighting, wget-like downloads, extensions, etc

I use it everyday to do REST calls 😄

@gdb
Copy link
Contributor Author

gdb commented Mar 27, 2016

HTTPie is great. Unfortunately, it's less about me developing my own API, and more about the external developers who are going to integrate against my API.

@hjacobs
Copy link
Contributor

hjacobs commented Apr 4, 2016

@jmcs @rafaelcaricio what is now the status here (apart from merge conflicts)? I think the trailing newline is OK (we already have indent as @gdb points out).

@rafaelcaricio
Copy link
Collaborator

@hjacobs I guess it is fine to add the new-line. In the future we could add configuration to Connexion to enable "human readable JSON responses", so it would enable the indentation + newline.

@hjacobs
Copy link
Contributor

hjacobs commented Apr 4, 2016

@gdb can you resolve the merge conflict to get this PR into master? Thanks!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.457% when pulling a6cc07c on gdb:master into 5e7c946 on zalando:master.

@gdb
Copy link
Contributor Author

gdb commented Apr 5, 2016

Ok!

@jmcs jmcs merged commit 40053ac into spec-first:master Apr 5, 2016
@hjacobs hjacobs mentioned this pull request Oct 12, 2016
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.

6 participants