Skip to content

Add GetTeamBySlug methods in teamsService - #1171

Merged
gmlewis merged 3 commits into
google:masterfrom
adrienzieba:add.GetTeamByName
May 30, 2019
Merged

Add GetTeamBySlug methods in teamsService#1171
gmlewis merged 3 commits into
google:masterfrom
adrienzieba:add.GetTeamByName

Conversation

@adrienzieba

Copy link
Copy Markdown
Contributor

Add a method to call the following endpoint
https://developer.github.com/v3/teams/#get-team-by-name

@googlebot

Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@codecov

codecovBot commented May 16, 2019

Copy link
Copy Markdown

Codecov Report

Merging #1171 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@## master #1171 +/- ##
==========================================
+ Coverage 70.2% 70.25% +0.05% 
==========================================
Files 84 84 Lines 5857 5867 +10 ==========================================
+ Hits 4112 4122 +10 
Misses 956 956 Partials 789 789
Impacted FilesCoverage Δ
github/teams.go71.75% <100%> (+1.69%)⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6264c79...36fb317. Read the comment docs.

@adrienzieba

adrienzieba commented May 17, 2019

Copy link
Copy Markdown
ContributorAuthor

My company (AppDirect) signed the CLA. I don't see the information on https://cla.developers.google.com/clas

@gmlewis

gmlewis commented May 17, 2019

Copy link
Copy Markdown
Collaborator

Hi @willnorris, when you get a chance, can you please check the status of the corporate CLA mentioned above? Also, just FYI... I will be OOO for one week starting tomorrow without internet access, so feel free to moderate as you see fit. @gauntface - just FYI for you too. Thanks!

@adrienzieba

Copy link
Copy Markdown
ContributorAuthor

CLA should be good now

@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebotgooglebot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels May 17, 2019

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @adrienzieba!

One change request, please, and one question that maybe @gauntface has an opinion about.

Comment threadgithub/teams.go Outdated
Comment threadgithub/teams.go Outdated
return t, resp, nil
}

// GetTeamByName fetches a team by slug.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since the parameter is called slug and your documentation also says slug, and the GitHub docs say "the team's slug", what do you think about calling this method GetTeamBySlug ?
I'm actually fine either way, but I had no clue what a slug was until I looked at the linked GitHub docs.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It was just to use the same name used as title inside the github documentation.
I'm fine changing the name of the method to GetTeamBySlug if needed.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Right... that makes total sense. Let's see what @gauntface thinks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay on this - GetTeamBySlug please.

The docs say: /orgs/:org/teams/:team_slug so I would definitely use slug, since it's different from the name.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@gauntface, @gmlewis I renamed the method. Thank you for your review.

@adrienzieba

Copy link
Copy Markdown
ContributorAuthor

Any update ?

@gmlewis

Copy link
Copy Markdown
Collaborator

@gauntface - do you have an opinion about my comment above?

@gauntfacegauntface left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Really sorry for the delay on this.

Comment threadgithub/teams.go Outdated
return t, resp, nil
}

// GetTeamByName fetches a team by slug.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay on this - GetTeamBySlug please.

The docs say: /orgs/:org/teams/:team_slug so I would definitely use slug, since it's different from the name.

@adrienziebaadrienzieba changed the title Add GetTeamByName methods in teamsServiceAdd GetTeamBySlug methods in teamsServiceMay 30, 2019

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @adrienzieba and @gauntface!
LGTM.

Awaiting second LGTM before merging.

@gauntfacegauntface left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 and apologies again for being sooo slow.

@gmlewis

Copy link
Copy Markdown
Collaborator

No problem, @gauntface! We greatly appreciate all your awesome reviews!
Merging.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yesIndication that the PR author has signed a Google Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@adrienzieba@googlebot@gmlewis@gauntface