Questions on PR review from Reddit event sources

This topic was automatically generated from Slack. You can find the original thread here.

SERW2039 : there are two comments I didnt’s addressed completely, one is the documentation on method. Why? Because in the samples I was given to follow there isn’t none. So I would want insisting from your part to implement? And I can start making going forward. And please let me address this with any other comments from the new set of changes

    ** This method retrieves the most recent new hot subreddit posts. The
    ** returned dataset contains at most `opts.limit` entries.
    ** @param {object}  opts options to customise the data retrieval
    ** @param {string}  opts.subreddit the subreddit from which to retrieve the
    ** hot posts
    ** @param {enum}    opts.region the region from where to retrieve the hot
    ** posts (e.g. `GLOBAL`, `US`, `AR`, etc.). See the `g` parameter in the
    ** docs for more information:
    ** @param {boolean} [opts.excludeFilters=false] if set to `true`, filters
    ** such as "hide links that I have voted on" will be disabled
    ** @param {boolean} [opts.includeSubredditDetails=false] whether the
    ** subreddit details should be expanded/included or not
    ** @param {number}  [opts.limit=100] the maximum amount of posts to retrieve

the other one change is letting the 400 error bubble up. So I removed the try/catch block, but no retry is done for 500, 429. I saw the example on Spotify and I can implement as i tis there. But I am not sure if here is a case where we actually want to retry? If that is the case please insist and I will do.

Jay Vercellone : For the documentation, the fact that not everything is documented is mostly historical and for the sake of speed.

Some methods are very simple and spending time documenting them doesn’t make much sense (e.g. documenting a method called createWebhook(httpEndpoint, eventType) doesn’t make sense as the interface is pretty explanatory itself).

Other methods are more complex, as they take multiple parameters/options of different types, their semantics is not straightforward, and it can take a lot of time from the next developer to understand. In those cases, documentation is super useful.

Here’s an example of a documented method: pipedream/base.js at ce20166cf451dad47fe6a209971d706af690fe1a · PipedreamHQ/pipedream · GitHub

During the code review, the meaning of the method was not clear, so adding documentation to it clarified this

Jay Vercellone : Regarding retries, let me look for an actual example of another event source

Jay Vercellone : pipedream/ at 3b9ab9310d25bd17429c92fb1d088696a38d8953 · PipedreamHQ/pipedream · GitHub

Jay Vercellone : It will of course depend on the specific API (see the _isRetriableStatusCode method). As a general rule, at the very least we should handle 429 status codes with an exponential backoff retry. Some API’

Jay Vercellone : Some API’s might return a 500 or 502 when they are experiencing a temporary glitch

Jay Vercellone : If we retry a few times and the issue is still there, then we can fail

SERW2039 : ah ok I see! So the documentation was added only to the new-hot-post-method, because it is the one that wasn’t that straight forward (hot post being more of a reddit term than of general knowledge compare to new link or comment), I included this documentation in that interface, plus a few parameters’s description were modified to match or be closer to each other… Took the chance and modified a couple of files more that were using the single quotation, that after the 500 issue when doing pd dev. Finally, I am implementing retry ongoing as per your comments, will revert.

SERW2039 : @UR84EMEUT @UMT4G7E5P guys I have changes pushed to the PR. I implemented the retry as specified by Jay, added the documentation comments to the one method for hot posts, performed code clean up from the review. Changes from the single quote from the weekend were pushed too. So when you have a chance please review! Did testing over all comments and everything look fine.

Dylan Sather (Pipedream) : great thank you Sergio, appreciate you making these changes and thank you and @UR84EMEUT for the help with the PR review. I’ll test these sources, as well

SERW2039 : I am done with changes a per the review. I will start testing, and expect to push in a few hours from now.
since i am close to wrapping this up, can I move to developing actions with the component model while the review is done? I see in the github list actions for coming up.

Dylan Sather (Pipedream) : We’re still working on the finishing touches of component actions, but for now could you tackle these Mailgun sources?

SERW2039 : @U019EEQLNSG @UR84EMEUT Hi, I have changes pushed to the PR as per lastest review. Could you guys have a look on chance? Sorry for the delay, there were issues found on my testing that I had to address, they are all fixed now. Changes include some issues in pagination, an async options was added to the subredditProp, other props were made optional as per the review.

SERW2039 : Moving onto Mailgun sources for now

Jay Vercellone : Sure! I’ll review it today