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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add helper #7

Closed
wants to merge 3 commits into from
Closed

Add helper #7

wants to merge 3 commits into from

Conversation

pmaslana
Copy link
Contributor

@pmaslana pmaslana commented May 2, 2024

No description provided.

Copy link
Contributor

@cmmarslender cmmarslender left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the changes requested, we should make this PR only about the abstraction of the pull request/team member function, and not add a bunch of empty files (or files with purely commented out code)

Comment on lines +11 to +12
// GetTeamMemList obtains a list of teammembers
func GetTeamMemList(githubClient *github.Client, internalTeam string) (map[string]bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// GetTeamMemList obtains a list of teammembers
func GetTeamMemList(githubClient *github.Client, internalTeam string) (map[string]bool, error) {
// GetTeamMemberList obtains a list of teammembers
func GetTeamMemberList(githubClient *github.Client, internalTeam string) (map[string]bool, error) {

More clear to just use full names. No penalties for a slightly longer function name here

},
owner := parts[0]
repo := parts[1]
teamMembers, _ := github2.GetTeamMemList(githubClient, internalTeam)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be inside of any loops - put it first thing in this function out of the loop. The team member list doesn't change based on what repo we're lookign at, and by doing it in the loop, we're rebuilding this list over and over for no gain (and a cost of a bunch of extra API calls and time)

break
}
if lowestNumber <= fullRepo.MinimumNumber {
break
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, I think we just need to continue instead of break so that we can look at the other PRs

@pmaslana pmaslana closed this May 2, 2024
@pmaslana pmaslana deleted the add-helper branch May 2, 2024 19:51
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.

2 participants