-
Notifications
You must be signed in to change notification settings - Fork 90
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
Add: Identify First-Time PR Contributors for Last Week #576
base: main
Are you sure you want to change the base?
Add: Identify First-Time PR Contributors for Last Week #576
Conversation
👷 Deploy request for leaderboard-develop pending review.Visit the deploys page to approve it
|
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.
instead of directly keeping the logic here, let's add it to the contributor object itself. so that this value isNewContributor
could be reused everywhere else when needed.
Also not a fan of the design. Let's not have |
Shall I add this logic in getContributorBySlug function? |
yup! that's the right place to be |
….com/JavidSumra/care_leaderboard into issues/153/highlight-new-contributor
….com/JavidSumra/care_leaderboard into issues/153/highlight-new-contributor
@rithviknishad can you please review this PR? |
const topContributors = [ | ||
...contributors.filter((contributor) => contributor.isNewContributor), | ||
...contributors | ||
.filter((contributor) => !contributor.isNewContributor) | ||
.slice(0, 8), | ||
]; |
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.
use Set instead to handle duplicates.
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.
Could you please explain why a set is needed in this case? We're simply adding the top contributors to highlight them for the week, right? Since the isNewContributor attribute prevents duplication, isn't that sufficient?
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.
sufficient yes, but it's just cleaner than too many spread operations and filters. more readable the code is, the better.
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.
I believe spread operators and filters are necessary since it’s an array of objects, and sets wouldn’t be usable in this case for the filtering process, right?
|
||
const firstActivity = activity | ||
.filter((act) => act.type === "pr_merged") | ||
.sort((a, b) => compareAsc(new Date(a.time), new Date(b.time)))[0]; |
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.
why wrap in date?
.sort((a, b) => compareAsc(new Date(a.time), new Date(b.time)))[0]; | |
.sort((a, b) => compareAsc(a.time, b.time))[0]; |
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.
@rithviknishad compareAsc
expects arguments of type Date
or number
, so I wrapped the value in a Date
instead of converting it from a string to a number.
Fixes #153
Description
This PR implements logic to identify first-time PR contributors whose first
pr_merged
activity occurred within the specified time frame (last week). It ensures accuracy by filtering contributors based on their activities and checks if their first contribution happened during last week.Screenshots / Outputs