Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feature to scan multiple paths at once #35

Merged
merged 4 commits into from
May 15, 2024

Conversation

chartgerink
Copy link
Contributor

@chartgerink chartgerink commented Jan 31, 2024

This PR adds the option to supply a vector of paths for the repo argument. This allows for creating contributor lists across a range of repositories at once (example: for an entire organization). This is in relation to #34.

In this PR the following is changed:

  • The contributor list generation per repo is factored out into a separate add_contributors_one_repo function
  • The add_contributors function now takes a vector of repo paths (but still works with only one path too)
  • If multiple repo's are added, the contributor list is deduplicated

Open questions for consideration are:

  • The link to the contributors issues or code are currently based on the last repo scanned. Is this okay or do we want to add links to all the contributed repo's?
  • The contribution counts are currently not summed but based on the first occurrence of the contributor. Is this okay or does it require summation of all contributions across all repo's provided?

I appreciate the opportunity to contribute - the above questions were a bit unforeseen from my end. I am open to any and all suggestions, also if that means ending up not including this code at all. Look forward to hearing what you think and what questions come up 馃槉

@mpadge
Copy link
Member

mpadge commented Jan 31, 2024

Thanks @chartgerink, looks awesome! I probably won't get time to have a good look until next week, unfortunately. In the meantime some responses to your questions:

The link to the contributors issues or code are currently based on the last repo scanned. Is this okay or do we want to add links to all the contributed repo's?

That was the only outstanding question i had thinking about this. I don't know an answer, and since you're the first person who has thought to use this pkg that way, I'm happy for you to decide what's best for you there.

The contribution counts are currently not summed but based on the first occurrence of the contributor. Is this okay or does it require summation of all contributions across all repo's provided?

Those counts are used to order the list of contributors, and i think it would likely be generally better to keep the ordering throughout. So yes, i think contributions should be summed. They're always extracted anyway, so doing that should be pretty straightforward.

I'd suggest replacing your current for rep in repo with an lapply(repo, function(rep) ...). You'd then have the full information including contribution counts which could be extracted from the first list item. Then you just do.call(rbind, result), remove duplicate contributions, and you should be good.

I'll look further next week. Thanks 馃殌

(You also should rename add_contributors_one_repo() to get_contributors_one_repo().)

@chartgerink
Copy link
Contributor Author

Thanks @mpadge 馃槉 I will take another pass and push an update this week so you have it before you take a deeper look.

I appreciate the clarity with which you are communicating your capacity - a week is still very fast!

@chartgerink
Copy link
Contributor Author

A bit later than promised, but I added the changes you suggested.

[Hardcoded repo links] was the only outstanding question i had thinking about this. I don't know an answer, and since you're the first person who has thought to use this pkg that way, I'm happy for you to decide what's best for you there.

Thanks for allowing me to make a suggestion 馃檹 I currently hardcoded the return of the organization in the first repository path provided, but happy to adjust. It doesn't look particularly approachable because of the x[, 'or'] already, so I'm happy to revise based on how we move forward.

PS: I'm coming back to R after several years of not actively developing so pardon my sloppiness. I'm still in NodeJS mode so wrangling some of the R objects is still a bit of a struggle. 馃槄

@mpadge mpadge mentioned this pull request May 15, 2024
@mpadge
Copy link
Member

mpadge commented May 15, 2024

Again, sorry @chartgerink that this ended up taking much longer than I originally suggested. I'll merge now, and then just make a couple of minor tweaks. All of your contributions will appear in an updated CRAN version asap. Thank you so much for your work, and engagement with the package - I really appreciate it!

@mpadge mpadge merged commit fc54467 into ropenscilabs:main May 15, 2024
6 checks passed
mpadge added a commit that referenced this pull request May 15, 2024
mpadge added a commit that referenced this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants