Draft: Add sponsorability feature#135
Conversation
…g Contributors[] type
kevindigo
left a comment
kevindigo
left a comment
There was a problem hiding this comment.
Let's discuss in slack or in a video call.
| public async fetchSponsorabilityInformation( | ||
| token: string, | ||
| repositoryIdentifiers: string[] | ||
| ): Promise<Map<string, ContributionCountOfUserIntoRepo[]>> { |
There was a problem hiding this comment.
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.
| repositoryIdentifiers: string[] | ||
| ): Promise<Map<string, ContributionCountOfUserIntoRepo[]>> { | ||
| //SponsorWithRepoIdAndContribution | ||
| const fetchAllContributors = new ContributorFetcher(this.config); |
There was a problem hiding this comment.
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.
not wild about the names but will change it
| await sponsorableContributorsFetcher.fetchSponsorableContributorsInformation( | ||
| token, | ||
| queryTemplate, | ||
| allContributorHistorys |
There was a problem hiding this comment.
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.
ok, passed it in to have the key of Contributor[],
| allContributorHistorys | ||
| ); | ||
|
|
||
| const sponsorableContributorWithContributonCounts = |
| }); | ||
| } | ||
|
|
||
| return sponsorables; |
There was a problem hiding this comment.
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 Sponsors? (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.
will work returning array, rename interface to Sponsorables
| contributor: SponsorableWithContributionCount; | ||
| }[] = []; | ||
|
|
||
| for (const [key, sponsors] of sponsorableContributor) { |
There was a problem hiding this comment.
It would be clearer to rename key to repoId. And sponsors should be sponsorables.
|
|
||
| public addContributionCount( | ||
| sponsorableContributor: Map<string, Sponsor[]>, | ||
| contributors: ContributionCountOfUserIntoRepo[] |
There was a problem hiding this comment.
This should probably be something like contributionCounts. I'm not sure that's right, but it's at least closer.
|
|
||
| /* | ||
|
|
||
| for each repo count the number of contributions that were made, |
There was a problem hiding this comment.
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.
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.
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.
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.
I was having issues trying to do that inside the loop, (was getting duplicates)
Co-authored-by: Kevin Smith <kevindigo@users.noreply.github.com>
…conversion function
2 participants
Work in progress