Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: separate request responsibility #12

Merged
merged 1 commit into from
Dec 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 38 additions & 64 deletions contributors.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@
* @example To archive contributors leaderboard data in csv file, run `node contributors.js`
*/

exports.archiveContributorsLeaderboard = archiveContributorsLeaderboard
exports.archiveContributorsLeaderboard = archiveContributorsLeaderboard;
exports.getAllRepos = getAllRepos;
exports.getAllContributors = getAllContributors;
exports.getRepoContributors = getRepoContributors;

const https = require('https');
const path = require('path');

const { makeRequest } = require('./network.js');

// Configurations (Optional)
// Repo owner that you want to analyze
const REPO_OWNER = process.env.REPO_OWNER;
Expand Down Expand Up @@ -40,37 +42,23 @@ async function getAllRepos(owner=REPO_OWNER, options) {
if(options && options.GITHUB_PERSONAL_TOKEN){
GITHUB_REQUEST_OPTIONS.headers["Authorization"] = "token "+options.GITHUB_PERSONAL_TOKEN;
Comment on lines 42 to 43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct modification of GITHUB_REQUEST_OPTIONS could lead to side effects. Consider creating a new options object for each request.

}
return new Promise((resolve, reject) => {
let url = `https://api.github.com/orgs/${owner}/repos?per_page=100&page=${pageNo}`;
console.log(url);
https.get(url, GITHUB_REQUEST_OPTIONS, (res) => {
console.log('statusCode:', res.statusCode);
// console.log('headers:', res.headers);
let data = '';
res.on('data', (d) => {
data += d;
})
res.on('end', async () => {
console.log("Repo list request finished")
// 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")
}
}
resolve(dataJsonArray);
})
}).on('error', (e) => {
console.error(e);
reject(e)
});
})
let url = `https://api.github.com/orgs/${owner}/repos?per_page=100&page=${pageNo}`;
const { res, data } = await makeRequest('GET', url, Object.assign({},GITHUB_REQUEST_OPTIONS));
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")
Comment on lines +55 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recursion for pagination could lead to stack overflow with many pages. An iterative approach may be more robust.

Also applies to: 82-85

}
Comment on lines +47 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider improving error handling beyond console logs to ensure robustness in production environments.

Also applies to: 75-86

}
Comment on lines +51 to +60
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return dataJsonArray;
}

/**
Expand All @@ -81,37 +69,23 @@ async function getAllRepos(owner=REPO_OWNER, options) {
* @example getRepoContributors('myorghandle/myreponame').then((contributors) => console.log(contributors)).catch((err) => console.log(err))
*/
async function getRepoContributors(fullRepoName, pageNo = 1) {
return new Promise((resolve, reject) => {
let url = `https://api.github.com/repos/${fullRepoName}/contributors?per_page=100&page=${pageNo}`;
console.log(url);
https.get(url, GITHUB_REQUEST_OPTIONS, (res) => {
console.log('statusCode:', res.statusCode);
// console.log('headers:', res.headers);
let data = '';
res.on('data', (d) => {
data += d;
})
res.on('end', async () => {
console.log("Contributors request finished for " + fullRepoName)
// 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 getRepoContributors(fullRepoName, pageNo);
dataJsonArray.push(...dataFromNextPage);
} catch (err) {
console.log("No more pagination needed")
}
}
resolve(dataJsonArray);
})
}).on('error', (e) => {
console.error(e);
reject(e)
});
})
let url = `https://api.github.com/repos/${fullRepoName}/contributors?per_page=100&page=${pageNo}`;
console.log(url);
const { res, data } = await makeRequest('GET', url, Object.assign({},GITHUB_REQUEST_OPTIONS));
console.log("Contributors request finished for " + fullRepoName)
// 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 getRepoContributors(fullRepoName, pageNo);
dataJsonArray.push(...dataFromNextPage);
} catch (err) {
console.log("No more pagination needed")
}
}
return dataJsonArray;
}

/**
Expand Down
51 changes: 51 additions & 0 deletions network.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
const https = require('https');

exports.makeRequest = makeRequest;

/**
* Sends an HTTPS request based on the specified method and options
* This function returns a Promise that resolves with the response and data received from the server
* @param {string} method - The HTTP method to use (e.g., 'GET', 'POST').
* @param {string} url - The URL to which the request is sent.
* @param {Object} [options] - The options for the request. This includes headers, request body (for POST/PUT), etc.
* @returns {Promise<{res: https.IncomingMessage, data: string}>} A Promise that resolves with the response object and the body data as a string.
* @throws {Error} Throws an error if the request cannot be completed
*
* @example
* // Example usage for a GET request within an async function
* async function getExample() {
* try {
* const { res, data } = await makeRequest('GET', 'https://example.com');
* console.log('Status Code:', res.statusCode);
* console.log('Body:', data);
* } catch (error) {
* console.error('Error:', error.message);
* }
* }
* */
async function makeRequest(method, url, options) {
return new Promise((resolve, reject) => {
// Ensure options is an object and set the method
options = typeof options === 'object' ? options : {};
options.method = method;

const req = https.request(url, options, res => {
// Handle HTTP response stream
let data = '';
res.on('data', chunk => data += chunk);
res.on('end', () => resolve({ res, data }));
});

req.on('error', error => {
console.error('Request error:', error);
reject(error);
});

// Handle POST/PUT data if provided
if (options.data) {
req.write(options.data);
}

req.end();
});
}
15 changes: 13 additions & 2 deletions test/contributors.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,24 @@ describe('contibutors.js', function() {

/** GitHub contrbutors test --START-- */

describe.skip('#getAllRepos('+contributorsFixture.VALID_REPO_OWNER+');', async function() {
it('should return the repos owned by '+contributorsFixture.VALID_REPO_OWNER, async function() {
this.timeout(100000);
let repos = await contributorsLib.getAllRepos(contributorsFixture.VALID_REPO_OWNER);
assert.isNotNull(repos);
expect(repos).to.be.an('array', 'Not an array')
expect(repos.length).to.be.greaterThanOrEqual(contributorsFixture.REPO_COUNT_MIN);
expect(repos[0]).to.include.all.keys('id', 'name', 'full_name', 'owner', 'private', 'html_url');
})
})

describe.skip('#getRepoContributors('+contributorsFixture.VALID_REPO+');', async function() {
it('should return the repo contribuors', async function() {
this.timeout(100000);
let contributors = await contributorsLib.getRepoContributors(contributorsFixture.VALID_REPO)
assert.isNotNull(contributors);
expect(contributors).to.be.an('array', 'Not an array')
expect(contributors.length).to.be.greaterThanOrEqual(contributorsFixture.REPO_CONTRIBUTOR_COUNT);
expect(contributors.length).to.be.greaterThanOrEqual(contributorsFixture.REPO_CONTRIBUTOR_COUNT_MIN);
expect(contributors[0]).to.include.all.keys('login','id','node_id','avatar_url','gravatar_id','url','html_url','followers_url','following_url','gists_url','starred_url','subscriptions_url','organizations_url','repos_url','events_url','received_events_url','type','site_admin','contributions');
})
})
Expand All @@ -24,7 +35,7 @@ describe('contibutors.js', function() {
let contributors = await contributorsLib.getAllContributors(contributorsFixture.VALID_REPO_OWNER);
assert.isNotNull(contributors);
expect(contributors).to.be.an('array', 'Not an array')
expect(contributors.length).to.be.greaterThanOrEqual(contributorsFixture.ALL_REPO_CONTRIBUTOR_COUNT);
expect(contributors.length).to.be.greaterThanOrEqual(contributorsFixture.ALL_REPO_CONTRIBUTOR_COUNT_MIN);
expect(contributors[0]).to.include.all.keys('login','id','node_id','avatar_url','gravatar_id','url','html_url','followers_url','following_url','gists_url','starred_url','subscriptions_url','organizations_url','repos_url','events_url','received_events_url','type','site_admin','contributions');
})
})
Expand Down
5 changes: 3 additions & 2 deletions test/fixtures/contributors.fixture.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
exports.VALID_REPO_OWNER = "Git-Commit-Show";
exports.VALID_REPO = "Git-Commit-Show/landing-page";
exports.REPO_CONTRIBUTOR_COUNT = 10;
exports.ALL_REPO_CONTRIBUTOR_COUNT = 49;
exports.REPO_COUNT_MIN = 10;
exports.REPO_CONTRIBUTOR_COUNT_MIN = 10;
exports.ALL_REPO_CONTRIBUTOR_COUNT_MIN = 49;
exports.VALID_CONTRIBUTOR_SAMPLE = {
login: "thenerdsuperuser",
id: 11832723,
Expand Down