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

Adapter for mongodb #58

Open
esakkiraj opened this issue May 12, 2017 · 9 comments
Open

Adapter for mongodb #58

esakkiraj opened this issue May 12, 2017 · 9 comments

Comments

@esakkiraj
Copy link

esakkiraj commented May 12, 2017

Hi

I am half way in writing a adapter for mongodb. Implementation is based on the given spec. Yet to published in npm.

Repo: https://github.com/esakkiraj/adapter-mongodb

@relekang
Copy link
Member

Cool, looks good 🎉

@mxstbr
Copy link
Member

mxstbr commented May 12, 2017

That's awesome, can't wait to see this finished!

@esakkiraj
Copy link
Author

Hi @relekang @mxstbr

Have almost done the implementation ( only two test's are failing, looking at it ). Meanwhile i thought there is mismatch in the specification and test for adapters for filter option.

Filter options before & after will be part of filter key in the options object. But in the adapter's test suite before and after key are passed in options object skipping the filter key.

Note: Currently i have managed to run the test by changing the implementation.

@mxstbr
Copy link
Member

mxstbr commented May 14, 2017

@relekang any comments? I'm totally not in the context here, haven't looked at that API in a while.

@relekang
Copy link
Member

Looks like the docs and implementation is the same. The implementation sends this as options.

{
  pathname: pathname,
  before: parseInt(query.before, 10),
  after: parseInt(query.after, 10),
}

@esakkiraj
Copy link
Author

Hi @relekang

As per the spec ( https://github.com/micro-analytics/micro-analytics-cli/blob/master/writing-adapters.md#options) filter is passed like this

await adapter.get('/hello', { filter: { after: 1200, before: 1234 }}) // -> { views: 20 }

but in the adapters-test suite the call is like

expect(await adapter.get('/a-key', { before: 1490623475640 })).toEqual({ views: [{ time: 1490623474639 }], });

I expected it to be

expect(await adapter.get('/a-key', { filter: { before: 1490623475640 } })).toEqual({ views: [{ time: 1490623474639 }], });

Don't know if i am missing something here.

@relekang
Copy link
Member

Yeah, that means that the spec differs from the implementation of micro-analytics-cli. If you follow the spec it will not work as expected, but if you follow the tests it will.

Not sure what we should do from here. If we should change the spec of change the implementation?

@mxstbr
Copy link
Member

mxstbr commented May 15, 2017

I think let's change the Spec, since it's easier to do? I don't think there's big of a difference between those two APIs...

@relekang
Copy link
Member

The spec is updated now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants