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 support for all create team parameters #683

Merged
merged 8 commits into from
Jan 27, 2020

Conversation

timja
Copy link
Collaborator

@timja timja commented Jan 25, 2020

Description

Add support for all create team parameters

Fixes #455

Implements all parameters from here: https://developer.github.com/v3/teams/#parameters

Before submitting a PR:

We love getting PRs, but we hate asking people for the same basic changes every time.

  • Push your changes to a branch other than master. Create your PR from that branch.
  • Add JavaDocs and other comments
  • Write tests that run and pass in CI. See CONTRIBUTING.md for details on how to capture snapshot data.
  • Run mvn -P ci install site locally. This may reformat your code, commit those changes. If this command doesn't succeed, your change will not pass CI.

Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

This looks really good.

Some name changes, tweaks, and questions. But high quality PR, thanks!

/**
* Creates a team.
*/
public class GHCreateTeamBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public class GHCreateTeamBuilder {
public class GHTeamBuilder {

Then we could also have a GHTeamUpdater.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

* the io exception
*/
public void setPrivacy(Privacy privacy) throws IOException {
root.createRequest().method("PATCH").with("privacy", privacy).withUrlPath(api("")).send();
Copy link
Member

Choose a reason for hiding this comment

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

@timja
This matches the existing behavior of other set methods on this class. There have been issues filed describing this is not great.

This is not a blocker for this PR, but I'd like your thoughts on #589.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree, changed to update.
I didn't like the name but I followed the other method for consistency in the class

Copy link
Member

Choose a reason for hiding this comment

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

Glad you agree. But leave this for now. I don't want to do this unevenly (some update some set). 😄

* the name
* @return the gh create repository builder
*/
public GHCreateTeamBuilder createTeam(String name) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the other createTeam methods should be marked as @Deprecated. Do you agree?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, wanted your input on it first before I did it, done.

import java.util.List;

/**
* Creates a team.
Copy link
Member

Choose a reason for hiding this comment

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

Add a link to the github api documentation for create team

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

* @return the gh create repository builder
*/
public GHCreateTeamBuilder createTeam(String name) {
return new GHCreateTeamBuilder(root, "/orgs/" + login + "/teams", name);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return new GHCreateTeamBuilder(root, "/orgs/" + login + "/teams", name);
return new GHCreateTeamBuilder(root, login, name);

Or maybe even this (and add name to builder):

Suggested change
return new GHCreateTeamBuilder(root, "/orgs/" + login + "/teams", name);
return new GHCreateTeamBuilder(root);

Is a team always created attached to a login? As me, could I create a team that your login owns?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did this for consistency with the create repo builder, I've made it take an org name now and not the 'apiTailUrl'.

I've kept the name as it's the only mandatory parameter for the builder

@timja timja requested a review from bitwiseman January 27, 2020 07:27
@@ -5,16 +5,18 @@

/**
* Creates a team.
*
* https://developer.github.com/v3/teams/#parameters
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* https://developer.github.com/v3/teams/#parameters
* https://developer.github.com/v3/teams/#create-team

*
* @param privacy
* the privacy
* @throws IOException
* the io exception
*/
public void setPrivacy(Privacy privacy) throws IOException {
public void updatePrivacy(Privacy privacy) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public void updatePrivacy(Privacy privacy) throws IOException {
public void setPrivacy(Privacy privacy) throws IOException {

@timja timja requested a review from bitwiseman January 27, 2020 07:40
@bitwiseman bitwiseman merged commit 57c4613 into hub4j:master Jan 27, 2020
@timja timja deleted the add-full-create-team-parameters branch January 27, 2020 08:02
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.

Add support for handling team's privacy
2 participants