-
Notifications
You must be signed in to change notification settings - Fork 7
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
Contribution cron job #70
base: main
Are you sure you want to change the base?
Contribution cron job #70
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.
This is a great start, but I don't think we did a good job of communicating the expectations for structure... I added some comments about using a library to help with the cron job stuff and the only other big change is that I think we should move the entirety of that endpoint out into utility functions and not expose them to the API. By that I mean we should probably have api/src/utils/github
files for each of the major commands, each with just the interfaces that relate to what they do, and then we should have a function within a new root level file (maybe something like src/cronJobs.ts
) that exports a single startCronJobs
method you call from index.ts
.
Let me know if you have any questions on the above; we can always hop on a call to chat. Beyond that, please don't write any more tests until we do another review of the cron stuff, just because I'm worried we'll need a few more rounds of revisions given the complexity of all this 😬
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.
Overall looks good to me, with just some small nit-picky changes for better clarity.
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.
LGTM 🚀
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.
Getting close! Awesome work; just a few more tweaks and then we can work on testing.
Pre-Requisites
Description of Changes
This PR allows for the continual fetching of user contribution data and saving it to our database
Related Issues