Skip to content

Improved format handling for the REST /api/rows and /api/changes#214

Merged
max-mapper merged 15 commits intodat-ecosystem:masterfrom
finnp:data-formats
Dec 4, 2014
Merged

Improved format handling for the REST /api/rows and /api/changes#214
max-mapper merged 15 commits intodat-ecosystem:masterfrom
finnp:data-formats

Conversation

@finnp
Copy link
Copy Markdown
Contributor

@finnp finnp commented Nov 12, 2014

This is just my WIP PR, which also will fail the tests until max-mapper/csv-write-stream#14 is merged and the data-format module is updated. It relates to bug #210

What this tries to achieve is having the exact same options for the REST api /api/rows endpoint as for createReadStream concerning the format option. Therefore I moved all formatting in its own module.

I also wanted to allow users to use accept headers, but that's the messy part in lib/format.js. There I am trying to map formats to mediatypes. Also I don't want to break the existing APIs. But maybe that is something worth doing?

/cc @Karissa

@finnp finnp changed the title WIP: REST Handling for /api/rows Improved format handling for the REST /api/rows and /api/changes Nov 16, 2014
@finnp
Copy link
Copy Markdown
Contributor Author

finnp commented Nov 17, 2014

I added format options for /api/changes and created some tests.

The /api/changes feed now defaults to json, however if live=true is set it uses ndjson instead, because json itself is not a suitable format for live streaming.

@okdistribute
Copy link
Copy Markdown
Collaborator

yeah i think that makes sense

@finnp
Copy link
Copy Markdown
Contributor Author

finnp commented Dec 2, 2014

I added a few more tests and I think it's ready to be reviewed/merged now.

@max-mapper
Copy link
Copy Markdown
Collaborator

i used the new version and added you as an owner!

@max-mapper
Copy link
Copy Markdown
Collaborator

oops wrong issue. i was talking about the collaborator module :P

@max-mapper max-mapper merged commit d7368fc into dat-ecosystem:master Dec 4, 2014
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.

3 participants