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

Get CSV response #253

Merged
merged 2 commits into from
Aug 15, 2015
Merged

Get CSV response #253

merged 2 commits into from
Aug 15, 2015

Conversation

diogob
Copy link
Contributor

@diogob diogob commented Aug 6, 2015

This is a draft, for I still need to make the accept headers work with text/csv + the API versioning.
But I'd like to get some early feedback on the code so far :D
This should fix issue #226

@diogob diogob changed the title Get CSV response [#226] Get CSV response Aug 6, 2015
@diogob
Copy link
Contributor Author

diogob commented Aug 6, 2015

BTW, the build is failing because of some problem with a hasql package during cabal exec packdeps step, but I haven't changed anything regarding the packages.

@begriffs
Copy link
Member

begriffs commented Aug 7, 2015

This will be a nice feature!

As for the test failure, try changing the version constraint in the cabal file to

hasql-postgres == 0.10.4.*

I should make packdeps a warning rather than an error because it breaks unpredictably when people update other packages.

@begriffs
Copy link
Member

begriffs commented Aug 8, 2015

I bumped the hasql-postgres version on master so when we merge this branch the packdeps will not complain.

Just tried out your feature and it's working great (nice refactors inside too)! Only thing I noticed is that the response headers say Content-Type: application/json even when it's returning csv in the body. Once that is fixed we should be good to merge.

@diogob
Copy link
Contributor Author

diogob commented Aug 9, 2015

Thanks man, I should be able to finish it in the next couple of days.

@jwarlander
Copy link

Really looking forward to this, let me know if I can help with any testing!

I'm looking at potentially using this for a data API that might at times be returning millions of rows, thus we're trying to avoid JSON.

@begriffs begriffs added ready and removed ready labels Aug 15, 2015
@begriffs begriffs merged commit 9d5011e into PostgREST:master Aug 15, 2015
@diogob
Copy link
Contributor Author

diogob commented Aug 15, 2015

@begriffs sorry, I was still working out some issues here, I'm opening a new PR for that ;)

@diogob
Copy link
Contributor Author

diogob commented Aug 15, 2015

And thanks for the neat mention in the README :D

monacoremo pushed a commit to monacoremo/postgrest that referenced this pull request Jul 17, 2021
monacoremo pushed a commit to monacoremo/postgrest that referenced this pull request Jul 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants