Keneucker/feature new updateInfo method #112
Conversation
Merging upstream develop
… file for this method The updateInfo method takes a required id and optional title and description parameters. 106
…ion for commitizen committizen -> commitizen
|
I'm also sneaking a typo change in the CONTRIBUTING.md file, "committizen" -> "commitizen". I missed that in my review of that PR, sorry! |
| extraFormParams.description = description; | ||
| } | ||
|
|
||
| return await imgur._imgurRequest('update', id, extraFormParams); |
There was a problem hiding this comment.
I'm not seeing where the update operation is being handled in imgur.js. It looks like this will throw an Invalid Operation error when it hits the default case in this switch block: https://github.com/kaimallea/node-imgur/blob/develop/lib/imgur.js#L82
Like the other operations, a match for update should prepare the options.method and options.url to adhere to what the Imgur API expects for an update (I'm referring to the Imgur API doc here: https://apidocs.imgur.com/#9d34ae67-9251-432c-9fe4-caad3fb46121).
Looks like it needs to be a POST request and hash is appended to the request url.
There was a problem hiding this comment.
I'm guessing something went haywire with the branching bc I see you updated the operation in a separate PR for the gallery. Did you forget to unstash? 😄
There was a problem hiding this comment.
@kaimallea I may have forgotten to unstash, or something. I have written this code several times in my upstream merges but I may have simply been confused. I'm updating this branch now.
| @@ -0,0 +1,63 @@ | |||
| const imgur = require('../lib/imgur.js'); | |||
There was a problem hiding this comment.
With the mocks in place, this test can be simplified, check the uploadFile for an example. But first I think the operation itself needs to be added
There was a problem hiding this comment.
I provided a good example in the gallery PR #113 (comment)
Check the msw docs for more deets/examples on how to write handlers to mock out: https://mswjs.io/docs/basics/request-handler
There was a problem hiding this comment.
@kaimallea I'm not sure what simplifications you were thinking of. I copied this test from search test, which included these other tests that I thought seemed appropriate enough. I've left all of the tests in there for now.
…or update operation Added the missing operation handling for 'update' and added mocked post request handler for updateInfo. #112
|
@kaimallea this one is ready for another review. |
|
Looks good, thanks! |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Adding the updateInfo method and accompanying test file.
This addresses an item from #63.
The upstream merge commits are in here because I created this branch on my forked version of the repository. @kaimallea, let me know if you'd prefer that I create branches on this repo, not my forked repository, for submitting PR's. It might be cleaner that way.