refactor: separate request responsibility #12
Conversation
WalkthroughThe codebase has undergone a refactoring to enhance modularity and error handling. A new function for repository retrieval was added, and existing functions were updated to use a centralized request handler, improving maintainability. Tests were adjusted to reflect these updates, including skipping some and updating expected values. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (4)
- contributors.js (3 hunks)
- network.js (1 hunks)
- test/contributors.test.js (2 hunks)
- test/fixtures/contributors.fixture.js (1 hunks)
Files skipped from review due to trivial changes (2)
- test/contributors.test.js
- test/fixtures/contributors.fixture.js
Additional comments: 6
contributors.js (5)
6-6: Semicolon added for consistency in export statements.
7-7: Addition of
getAllReposto the exports supports the centralization of network requests.13-13: Import of
makeRequestfromnetwork.jsreplaces directhttpsusage, aligning with the centralization goal.45-61: Refactored
getAllReposto usemakeRequestfor network calls, which is a core part of the centralization effort.72-88: Refactored
getRepoContributorsto usemakeRequestfor network calls, which is consistent with the centralization goal.network.js (1)
- 1-51: The
makeRequestfunction is well-implemented with proper error handling and usage of Promises for asynchronous operations. The documentation and example usage provided are clear and helpful. Ensure that the rest of the codebase is updated to use this new function for all network requests.
| console.log("Repo list request finished"); | ||
| console.log('HTTP status: ', res.statusCode); | ||
| // console.log(data) | ||
| let dataJsonArray = JSON.parse(data); | ||
| if (dataJsonArray.length == 100) { | ||
| //It might have more data on the next page | ||
| pageNo++; | ||
| try { | ||
| let dataFromNextPage = await getAllRepos(owner, { pageNo: pageNo } ); | ||
| dataJsonArray.push(...dataFromNextPage); | ||
| } catch (err) { | ||
| console.log("No more pagination needed") | ||
| } |
There was a problem hiding this comment.
Consider improving error handling beyond console logs to ensure robustness in production environments.
Also applies to: 75-86
| if (dataJsonArray.length == 100) { | ||
| //It might have more data on the next page | ||
| pageNo++; | ||
| try { | ||
| let dataFromNextPage = await getAllRepos(owner, { pageNo: pageNo } ); | ||
| dataJsonArray.push(...dataFromNextPage); | ||
| } catch (err) { | ||
| console.log("No more pagination needed") | ||
| } | ||
| } |
There was a problem hiding this comment.
The pagination logic assumes that a full page of results indicates more data. It may be helpful to add a comment explaining this assumption for future maintainability.
Also applies to: 78-87
| if(options && options.GITHUB_PERSONAL_TOKEN){ | ||
| GITHUB_REQUEST_OPTIONS.headers["Authorization"] = "token "+options.GITHUB_PERSONAL_TOKEN; |
There was a problem hiding this comment.
Direct modification of GITHUB_REQUEST_OPTIONS could lead to side effects. Consider creating a new options object for each request.
| let dataFromNextPage = await getAllRepos(owner, { pageNo: pageNo } ); | ||
| dataJsonArray.push(...dataFromNextPage); | ||
| } catch (err) { | ||
| console.log("No more pagination needed") |
There was a problem hiding this comment.
Recursion for pagination could lead to stack overflow with many pages. An iterative approach may be more robust.
Also applies to: 82-85
A step before developing rate limit abilities - get all requests to be handled by single file
network.jsSummary by CodeRabbit