brettlangdon / node-dogapi

Datadog API Node.JS Client
https://brettlangdon.github.io/node-dogapi/
105 stars 45 forks source link

Embed api #34

Closed joshkurz closed 8 years ago

joshkurz commented 8 years ago
brettlangdon commented 8 years ago

Test should fail, this file will need to be updated (renamed to embed.js and imports/names fixed): https://github.com/brettlangdon/node-dogapi/blob/e58e06bfa8f4fde1262e87cb4970da5600f28468/test/api/graph.js

brettlangdon commented 8 years ago

other than that, quick glance it looks decent, I won't be able to give it an in-depth look until later tonight or tomorrow morning

brettlangdon commented 8 years ago

lastly... thanks again for your contribution!!!!!! :)

joshkurz commented 8 years ago

still needs tests and things. I have tested this locally and all commands work fine, from command line and from my application.

brettlangdon commented 8 years ago

@joshkurz sorry it took me so long to get around to it, but after my comments, this PR should be good to go.

joshkurz commented 8 years ago

updated and rebased

brettlangdon commented 8 years ago

I had one final thought here, can we link dogapi.graph.createEmbed to dogapi.embed.create, this way we don't have to bump major versions for this release.

e.g.

// graph.js
var embed = require('../embed');

module.exports = {
    createEmbed: embed.create,
    ...
};

Thoughts?

joshkurz commented 8 years ago

yeah that's a good idea.

joshkurz commented 8 years ago

I added the graph.js tests back also.

brettlangdon commented 8 years ago

If you drop the graph createEmbed tests, then lgtm

joshkurz commented 8 years ago

dont you want to make sure its backwards compatible?

brettlangdon commented 8 years ago

Sure, but we shouldn't need to have duplicate tests to ensure that. We could add a test to assert that dogapi.graph.createEmbed exists, but probably not worth having a test just for that.

joshkurz commented 8 years ago

ok well they passed, I honestly just wanted to test to make sure it works at least once to be sure.

brettlangdon commented 8 years ago

:+1: great, one these latest tests pass, I'll merge and release right away!

Thanks your your contribution! I really appreciate the work!