Add Code Examples #18
Conversation
There was a problem hiding this comment.
These are definitely helpful examples! Left some style / cleanliness comments.
Are these intended to be used off the shelf? If so then we should change scripts to take command-line arguments.
Also you should use a linter to check for lint errors that I may have missed.
examples/cancelTasks.js
Outdated
| // Process each row as needed | ||
| rows = rows.map( | ||
| row => row.split(',')[0].trim() | ||
| ); |
There was a problem hiding this comment.
It would make more sense for splitting by ',' should be handled by readCsv()
There was a problem hiding this comment.
Might want to use a csv library. split(',') fails when you have string values that contain commas 😉
There was a problem hiding this comment.
Good point - csv-parse is a good one that I use in my own scripts!
There was a problem hiding this comment.
I updated the logic a bit to handle this better, I'd like to try and not add any more packages to the package.json beyond request and I don't think fancy .csv handling is worth the extra dev hassle to get value from this script. If it's cool with you guys, I'll leave my little helper function here instead of doing the fully correct solution.
There was a problem hiding this comment.
I don't agree with that. You're providing a csv parser that doesn't handle edge cases. Why does the example need CSV? Why not just use a text file with task ids on new lines? I would also expect any service or API we provide to correctly handle input validation (e.g., testing id string length).
There was a problem hiding this comment.
I'll go with the .txt file approach, that seems really reasonable.
I think somewhere else I had input validation, I'll add that here as well.
There was a problem hiding this comment.
Sorry, I meant the API side should handle it so the customers shouldn't have to worry about it
| // === CANCEL TASKS === | ||
| // ==================== | ||
|
|
||
| await Promise.map( |
There was a problem hiding this comment.
This is an external library called Bluebird that our environment automatically monkey patches in. There is no Promise.map in node.js
There was a problem hiding this comment.
Ah!! This makes SO much more sense. I thought my google-fu was just off and I was missing something but couldn't explain how this worked otherwise. Will update.
|
@shaun-scale James and I probably are not the best people to review this. You should go with the team that maintains the API. |
No description provided.