-
Notifications
You must be signed in to change notification settings - Fork 19
Draft: Add sponsorability feature #135
base: main
Are you sure you want to change the base?
Conversation
…g Contributors[] type
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.
Let's discuss in slack or in a video call.
src/sponsorabilityFinder.ts
Outdated
public async fetchSponsorabilityInformation( | ||
token: string, | ||
repositoryIdentifiers: string[] | ||
): Promise<Map<string, ContributionCountOfUserIntoRepo[]>> { |
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 think I would expect the return value from this function to be an array, where each entry contains all of the data for one row of the spreadsheet.
src/sponsorabilityFinder.ts
Outdated
repositoryIdentifiers: string[] | ||
): Promise<Map<string, ContributionCountOfUserIntoRepo[]>> { | ||
//SponsorWithRepoIdAndContribution | ||
const fetchAllContributors = new ContributorFetcher(this.config); |
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.
The name should be a noun, like contributorFetcher
. Or, since later we learn that it's returning histories, maybe the class should be ContributorHistoryFetcher
and this const should be contributorHistoryFetcher
?
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.
not wild about the names but will change it
src/sponsorabilityFinder.ts
Outdated
await sponsorableContributorsFetcher.fetchSponsorableContributorsInformation( | ||
token, | ||
queryTemplate, | ||
allContributorHistorys |
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.
Generally it would be better to only pass in the needed information. In this case, passing just a simple list of contributors (without history).
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.
ok, passed it in to have the key of Contributor[],
src/sponsorabilityFinder.ts
Outdated
allContributorHistorys | ||
); | ||
|
||
const sponsorableContributorWithContributonCounts = |
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 like this step
}); | ||
} | ||
|
||
return sponsorables; |
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.
Since we already have a map of contributors by repo from elsewhere, does this need to tie the sponsorables back to their repos? Could this just return an array of Sponsor
s? (Which I would rename Sponsorable
, since these are the people potentially being sponsored, not the people doing the sponsoring.)
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.
will work returning array, rename interface to Sponsorables
src/sponsorabilityFinder.ts
Outdated
contributor: SponsorableWithContributionCount; | ||
}[] = []; | ||
|
||
for (const [key, sponsors] of sponsorableContributor) { |
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.
It would be clearer to rename key
to repoId
. And sponsors
should be sponsorables
.
src/sponsorabilityFinder.ts
Outdated
|
||
public addContributionCount( | ||
sponsorableContributor: Map<string, Sponsor[]>, | ||
contributors: ContributionCountOfUserIntoRepo[] |
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 should probably be something like contributionCounts
. I'm not sure that's right, but it's at least closer.
src/sponsorabilityFinder.ts
Outdated
|
||
/* | ||
|
||
for each repo count the number of contributions that were made, |
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.
More of this kind of comment might be helpful while the naming and logic is still being worked out. Then, by the end, hopefully the code will be clear enough that the comments will be redundant and can be deleted.
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.
yes, planning to remove all comments once my logic and naming is fleshed out
const allSponsorableUsersInfo = []; | ||
const sponsorables = new Map<string, Sponsor[]>(); | ||
|
||
for (const [repoIdentifier, githubUsers] of contributors.entries()) { |
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.
To avoid dealing with "entries", I would typically have this outer loop iterate through just the "keys". Then the first line inside the loop would be const users = contributors.get(repoIdentifier);
} | ||
} | ||
|
||
// need to refactor: |
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 suspect this would be simpler if you built the map inside the loop above, instead of building it there an array, and then converting it to a map here.
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 was having issues trying to do that inside the loop, (was getting duplicates)
Co-authored-by: Kevin Smith <[email protected]>
…conversion function
Work in progress