Handle remove by rule ids #38
Conversation
|
? |
|
Do you mind adding the method to the Readme? Also, thanks for this and we're going to use your fork. |
I'll add in the readme. and I'm also using the fork for now, but I do hope this would get merged |
|
Can someone please test if this works? I cannot merge untested changes and I'm not using this lib anymore. A bunch of unit tests will be good and greatly appreciated. |
|
Thanks @demian85 are there unit tests in this lib? I failed to find them. Can you point me and I'll write one? |
|
Do you mean just tell you if it works or write tests for it? I can tell you that it works because I'm using right now. |
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "gnip", | |||
| "version": "2.2.3", | |||
| "version": "2.2.4", | |||
There was a problem hiding this comment.
What about version in package.json? They should match.
There was a problem hiding this comment.
This was bumped in my side because I use this as a stop gap until this pr is merged And bumped when npm version -
When you do an npm version on your side you'll bump it.. or just edit the PR, if I do it in my side I'll break my existing code
Another way around this would be that I would bump to version 2.2.5 -
What would you prefer?
|
Any updates on this PR? would be handy to have delete by ID in my current project. |
|
@thisispete for now you can use my branch which I use with delete by id for a long time |
| request.post({ | ||
| url: this._api + '?_method=delete', | ||
| json: json, | ||
| headers: { 'Authorization': this._auth } | ||
| }, function (err, response, body) { | ||
| if (err) cb(err); | ||
| else if (response.statusCode >= 200 && response.statusCode < 300) cb(null, body); | ||
| else cb(new Error('Unable to delete rules. Request failed with status code: ' + response.statusCode + 'body: '+ JSON.stringify(body))); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Can you extract this logic so we can reuse it in the method above? It is the same, except the payload is different. After this change, I can merge and release a new minor version later today,.
|
Please check my comment above and resolve conflicts. |
Also previous commits to handle error logging, mostly became unneeded with changes in the lib