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
Store a list of current repos which use the JSON Schema topic and when they were created #4 #10
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome, and thanks for your initial work on this.
I have left a few comments and questions.
Looking at the requirements on #4.
- Covered.
- I have updated this to specify using the timestamp format.
- Not covered. "Fully run" means all results without limit. There are about 1.8k results. This may take some time to run.
- Not covered. This is important because it can take some time to run, so the script should capture errors and continue running rather than crash and exit.
When you have updated and tested, please indicate this needs to be re-reviewed. Thanks.
Feel free to reach me on our Slack under the same username if you have questions.
deleted unnecessary file
This is my second qualification task work! |
This looks much improved. I'm scheduling some time to test it locally before it is merged. Thanks for your work on this. |
deleted unnecessary file
I rebased and force pushed to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few suggestions, otherwise this is looking better.
I know GSoC candidates have been selected now, but it would be nice to know if you'd like to finish this PR or not. If not, that's totally fine, and I'm sure I can find someone else to finish it or do it myself. Let me know.
projects/initial-data/main.js
Outdated
return Date.parse(lastPageResponse.data[0].commit.author.date); | ||
} else { | ||
console.error('Error occured'); | ||
throw new Error('No commits found'); //TODO: check if this is the correct error message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new Error('No commits found'); //TODO: check if this is the correct error message | |
throw new Error(`No commits found ${owner}/${repo}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed as suggested
projects/initial-data/main.js
Outdated
} | ||
} catch (err) { | ||
console.log(err); | ||
throw new Error('No commits found'); //TODO: check if this is the correct error message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new Error('No commits found'); //TODO: check if this is the correct error message | |
throw new Error(`Could not find any commits for ${owner}/${repo}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed as suggested
projects/initial-data/main.js
Outdated
if (response.data.length > 0) { | ||
response.data[0].created_at; | ||
} else { | ||
throw new Error('No releases found'); //TODO: check if this is the correct error message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new Error('No releases found'); //TODO: check if this is the correct error message | |
throw new Error(`No releases found for ${owner}/${repo}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed as suggested
projects/initial-data/main.js
Outdated
if (lastPageResponse.data.length > 0) { | ||
return Date.parse(lastPageResponse.data[0].created_at); | ||
} else { | ||
throw new Error('No releases found'); //TODO: check if this is the correct error message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new Error('No releases found'); //TODO: check if this is the correct error message | |
throw new Error(`No releases found for ${owner}/${repo}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed as suggested
projects/initial-data/main.js
Outdated
} | ||
} catch (err) { | ||
console.error('Error occured'); | ||
throw new Error('No releases found'); //TODO: check if this is the correct error message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new Error('No releases found'); //TODO: check if this is the correct error message | |
throw new Error(`Unable to get releases for ${owner}/${repo}`); |
projects/initial-data/main.js
Outdated
firstReleaseDate = await fetchFirstReleaseDate(octokit, owner, repo); | ||
} catch (err) { | ||
console.error('Error occured in fetchFirstReleaseDate'); | ||
throw new Error('No releases found'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new Error('No releases found'); | |
throw new Error(`Unable to get releases for ${owner}/${repo}`); |
projects/initial-data/main.js
Outdated
try { | ||
firstCommitDate = await fetchFirstCommitDate(octokit, owner, repo); | ||
} catch (err) { | ||
console.error('Error occured in fetchFirstCommitDate'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.error('Error occured in fetchFirstCommitDate'); | |
console.error(`Error occured in fetchFirstCommitDate for ${owner}/${repo}`); |
projects/initial-data/main.js
Outdated
firstCommitDate = await fetchFirstCommitDate(octokit, owner, repo); | ||
} catch (err) { | ||
console.error('Error occured in fetchFirstCommitDate'); | ||
throw new Error('No commits found'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new Error('No commits found'); | |
throw new Error(`Error trying to find first commit for ${owner}/${repo}`); |
[after fixed] Hi Ben, Thank you for your insightful feedback! I appreciate the guidance and the opportunity to learn from real-world problems provided by this project. I've completed a git rebase and resolved the divergent commits as you've noticed. I've also re-requested a review of the PR to ensure everything aligns with the project's current needs. Regarding the API key update, your clarification was very helpful. I understand now the importance of designing tests that don't make real API calls and will take this as a key learning point. Thank you for your support and for providing a nurturing environment for newcomers like me. I look forward to continuing my contribution to the project and will keep in touch via Slack as needed. Best regards, |
Resolves #4
This PR is for 'Store a list of current repos which use the JSON Schema topic and when they were created
#4'
Requirement
What I have done
This commit includes several enhancements to the initial data script: