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

Adding pretty JSON #262

Closed
wants to merge 1 commit into from
Closed

Adding pretty JSON #262

wants to merge 1 commit into from

Conversation

obihann
Copy link

@obihann obihann commented Aug 20, 2015

  • addition of prettyjson to src/prettyify-response.coffee so that body and schema objects that are of content type `application/json' are easier to read
  • update to how we check content type, since types such as application/json; charset=utf-8 are valid we should not do an exact match on application/json and instead do an indexOf
  • allowing schemaBody to be passed through prettyjson along with schema

+ addition of [prettyjson](https://github.com/rafeca/prettyjson) to `src/prettyify-response.coffee` so that body and schema objects that are of content type `application/json' are easier to read
+ update to how we check content type, since types such as `application/json; charset=utf-8` are valid we should not do an exact match on `application/json` and instead do an indexOf
@honzajavorek
Copy link
Contributor

@obihann I'm sorry we did not respond for a longer time 😰 Thanks for your work! 👍 As master is fixed now, would you mind to rebase with it and try whether tests pass?

@honzajavorek
Copy link
Contributor

And since you're already touching the content type logic in your PR, I'd prefer if it would be able to deal also with content types such as application/hal+json, application/vnd.geo+json and similar ones. What do you think?

@obihann
Copy link
Author

obihann commented Jan 4, 2016

@honzajavorek sure, I'll rebase this and see how it goes. I'll also try to add those content types in.

@honzajavorek
Copy link
Contributor

@obihann Awesome! If I may provide hints, I'd use media-typer for the heavy lifting.

@netmilk
Copy link
Contributor

netmilk commented Jan 4, 2016

Looking forward @obihann! Thanks a lot for all your contributions!

@honzajavorek
Copy link
Contributor

I think we should incorporate this as part of #765.

@honzajavorek
Copy link
Contributor

I'm very sorry we were not able to get back to this. I need to close this as we're going to work on #705 first, which requires having no pending Pull Requests. We'll get back to this proposal as soon as we're revamping the CLI reporter: #765

Thanks for all the work! I'll do my best for this contribution not to be forgotten!

@revett
Copy link

revett commented Mar 15, 2018

@honzajavorek Have you come back to this feature yet?

@honzajavorek
Copy link
Contributor

@revett We thought about revamping the whole CLI reporter: #765

@revett
Copy link

revett commented Mar 15, 2018

@honzajavorek Ah okay 👍That seems like quite a large piece of work but this issue is a simple fix. If I put in a PR for this, would it be merged before #765?

@honzajavorek
Copy link
Contributor

Yes, that would be nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic: CLI reporter Dredd's default reporter in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants